Skip to content

[FrameworkBundle] ensureKernelShutdown in tearDownAfterClass #60564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: 6.4
Choose a base branch
from

Conversation

cquintana92
Copy link

@cquintana92 cquintana92 commented May 27, 2025

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues None
License MIT

This PR performs the following change: In KernelTestCase, the kernel is shut down after every test, as per the static::ensureKernelShutdown() call in the tearDown function. However, if one was to perform any extra code in their tearDownAfterClass and call getContainer, it would boot up a new kernel, which would then be left dangling. The main issue with that is that when the next Test class is executed, there would already be a booted kernel, so it could happen that it was pointing to dangling resources.

By adding this call to static::ensureKernelShutdown in the tearDownAfterClass, we can ensure that no dangling kernels are left even after the test class has finished.

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@cquintana92 cquintana92 force-pushed the chore/ensure-kernel-shutdown-teardownafterclass branch from ea858f6 to feeac45 Compare May 27, 2025 13:09
@OskarStark OskarStark changed the title chore: ensureKernelShutdown in tearDownAfterClass ensureKernelShutdown in tearDownAfterClass May 27, 2025
@carsonbot carsonbot changed the title ensureKernelShutdown in tearDownAfterClass [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass May 27, 2025
@nicolas-grekas
Copy link
Member

This relies on the child class calling the parent class.
I'd suggest using the @afterClass annotation instead (and making the method public).
This will provide better ordering also because this methods with annotation are called after tearDownAfterClass

@cquintana92
Copy link
Author

@nicolas-grekas I've added it in a new public method annotated with @afterClass. If you prefer I can just annotate the ensureKernelShutdown method with @afterClass, but I found cleaner to split the actual logic from the test lifecycle event.

@nicolas-grekas nicolas-grekas modified the milestones: 7.4, 6.4 May 30, 2025
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd make more sense to me to add the annotation on the existing method.
My concern is about increasing the public API. People will start using the new method instead of the existing one in their test cases / IDEs will propose both, and people will wonder about that.

Also: I'm fine merging this as a bugfix, so 6.4 works for me.

@cquintana92
Copy link
Author

cquintana92 commented Jun 2, 2025

Made the change to set the annotation directly in the ensureKernelShutdown method.

@nicolas-grekas nicolas-grekas changed the base branch from 7.4 to 6.4 June 2, 2025 06:44
@nicolas-grekas nicolas-grekas force-pushed the chore/ensure-kernel-shutdown-teardownafterclass branch 2 times, most recently from 674990e to 8d81b0d Compare June 2, 2025 06:45
@cquintana92
Copy link
Author

@nicolas-grekas looks like by changing the method to @afterClass the Unit Tests check for 8.3 fails on

Symfony\Component\HttpClient\Tests\AmpHttpClientTest::testNonBlockingStream

Do you think it could be related to the test (actually the HttpClientTestCase base class) doing work that assumes a non-empty buffer?
I'm not well-versed enough in the Symfony codebase to understand why it would work with a new method marked with @afterClass but not by marking ensureKernelShutdown as @afterClass

@nicolas-grekas
Copy link
Member

That failure is a false-positive so we can ignore it.

@@ -120,8 +120,10 @@ protected static function createKernel(array $options = []): KernelInterface

/**
* Shuts the kernel down if it was used in the test - called by the tearDown method by default.
*
* @afterClass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should also have the attribute, to be compatible with PHPUnit 10+

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've added the attribute in addition of the annotation 👍
Is there any other change required?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof looks like the attribute is not available, Psalm fails but the tests pass... Is there anything I missed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the attribute is not available because Psalm runs with PHPUnit 9.x installed. But that's not an issue. We don't treat Psalm as a mandatory check, only as an helper tool (and each PR reports only new Psalm errors)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for the answer!

@cquintana92 cquintana92 force-pushed the chore/ensure-kernel-shutdown-teardownafterclass branch from 7b1d5ad to bf9786c Compare June 3, 2025 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants