[FrameworkBundle] ensureKernelShutdown in tearDownAfterClass #60564

cquintana92 · 2025-05-27T13:03:16Z

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 · 2025-05-27T13:03:22Z

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

nicolas-grekas · 2025-05-30T08:22:34Z

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 · 2025-05-30T13:09:12Z

@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

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 · 2025-06-02T06:37:10Z

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

cquintana92 · 2025-06-02T07:43:43Z

@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 · 2025-06-02T09:09:09Z

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

stof · 2025-06-02T10:08:26Z

src/Symfony/Bundle/FrameworkBundle/Test/KernelTestCase.php

@@ -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


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

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

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

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)

Okay, thanks for the answer!

nicolas-grekas · 2025-06-04T08:56:42Z

Thank you @cquintana92.

Seldaek · 2025-06-04T13:26:10Z

So this merge broke our build because for performance reason (to avoid recreating containers at every test) we suppress the tearDown client reset behavior, explicitly not calling the parent::tearDown(), and then suddenly the client got reset anyway, causing issues.

Not sure what to do about this though.. I can see how this fixes a bug too.

nicolas-grekas · 2025-06-04T13:40:24Z

I realized that there's another issue: we made the method public, which will break classes that override the method but keep it protected.
Let's revert...

…AfterClass (cquintana92)" This reverts commit ec76ab4, reversing changes made to bc88600.

nicolas-grekas · 2025-06-04T13:57:58Z

Reverted in 0291ea1

cquintana92 · 2025-06-04T14:10:22Z

How should we proceed to include this, then? Will another PR be made for including the call to ensureKernelShutdown?

nicolas-grekas · 2025-06-04T14:16:20Z

I suggest going with your initial PR: calling ensureKernelShutdown in teardownAfterClass
Can you submit it?
Then let's discuss about possible impacts there.

cquintana92 · 2025-06-04T14:51:06Z

@nicolas-grekas there you go: #60693

…ss (cquintana92) This PR was submitted for the 7.4 branch but it was squashed and merged into the 6.4 branch instead. Discussion ---------- [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | None | License | MIT Comes from #60564 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. Commits ------- c193b98 [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass

* 6.4: cs tweak [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass Revert "bug #60564 [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass (cquintana92)"

* 7.2: cs tweak [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass Revert "bug symfony#60564 [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass (cquintana92)"

* 7.3: [HttpClient] Suggest amphp/http-client v5 by default [WebProfilerBundle] Fix typos in routing config deprecation messages cs tweak [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass Revert "bug symfony#60564 [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass (cquintana92)" [Security] Keep roles when serializing tokens [JsonPath] Always use brackets notation with `JsonPath::key()`

* 7.4: [HttpClient] Suggest amphp/http-client v5 by default [Security] conflict with event-subscriber v8 Fix leftover [WebProfilerBundle] Fix typos in routing config deprecation messages cs tweak [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass Revert "bug symfony#60564 [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass (cquintana92)" [Security] Keep roles when serializing tokens [JsonPath] Always use brackets notation with `JsonPath::key()`

carsonbot added Status: Needs Review Bug labels May 27, 2025

carsonbot added this to the 7.4 milestone May 27, 2025

cquintana92 force-pushed the chore/ensure-kernel-shutdown-teardownafterclass branch from ea858f6 to feeac45 Compare May 27, 2025 13:09

OskarStark changed the title ~~chore: ensureKernelShutdown in tearDownAfterClass~~ ensureKernelShutdown in tearDownAfterClass May 27, 2025

OskarStark added the FrameworkBundle label May 27, 2025

carsonbot changed the title ~~ensureKernelShutdown in tearDownAfterClass~~ [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass May 27, 2025

nicolas-grekas modified the milestones: 7.4, 6.4 May 30, 2025

nicolas-grekas reviewed May 30, 2025

View reviewed changes

nicolas-grekas changed the base branch from 7.4 to 6.4 June 2, 2025 06:44

nicolas-grekas requested review from lyrixx, yceruto, chalasr, dunglas, OskarStark, jderusse and xabbuh as code owners June 2, 2025 06:44

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

nicolas-grekas approved these changes Jun 2, 2025

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 2, 2025

stof reviewed Jun 2, 2025

View reviewed changes

[FrameworkBundle] ensureKernelShutdown in tearDownAfterClass

bf9786c

cquintana92 force-pushed the chore/ensure-kernel-shutdown-teardownafterclass branch from 7b1d5ad to bf9786c Compare June 3, 2025 10:50

stof approved these changes Jun 3, 2025

View reviewed changes

nicolas-grekas merged commit ec76ab4 into symfony:6.4 Jun 4, 2025
10 of 11 checks passed

nicolas-grekas added a commit that referenced this pull request Jun 4, 2025

Revert "bug #60564 [FrameworkBundle] ensureKernelShutdown in tearDown…

0291ea1

…AfterClass (cquintana92)" This reverts commit ec76ab4, reversing changes made to bc88600.

cquintana92 mentioned this pull request Jun 4, 2025

[FrameworkBundle] ensureKernelShutdown in KernelTestCase::tearDownAfterClass #60693

Merged

nicolas-grekas added a commit that referenced this pull request Jun 5, 2025

Merge branch '6.4' into 7.2

959b0ff

* 6.4: cs tweak [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass Revert "bug #60564 [FrameworkBundle] ensureKernelShutdown in tearDownAfterClass (cquintana92)"

Uh oh!

[FrameworkBundle] ensureKernelShutdown in tearDownAfterClass #60564

[FrameworkBundle] ensureKernelShutdown in tearDownAfterClass #60564

Conversation

cquintana92 commented May 27, 2025 • edited by OskarStark Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonbot commented May 27, 2025

Uh oh!

nicolas-grekas commented May 30, 2025

Uh oh!

cquintana92 commented May 30, 2025

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

cquintana92 commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cquintana92 commented Jun 2, 2025

Uh oh!

nicolas-grekas commented Jun 2, 2025

Uh oh!

stof Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

cquintana92 Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

cquintana92 Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

stof Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

cquintana92 Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jun 4, 2025

Uh oh!

Uh oh!

Seldaek commented Jun 4, 2025

Uh oh!

nicolas-grekas commented Jun 4, 2025

Uh oh!

nicolas-grekas commented Jun 4, 2025

Uh oh!

cquintana92 commented Jun 4, 2025

Uh oh!

nicolas-grekas commented Jun 4, 2025

Uh oh!

cquintana92 commented Jun 4, 2025

Uh oh!

Uh oh!

cquintana92 commented May 27, 2025 •

edited by OskarStark

Loading

cquintana92 commented Jun 2, 2025 •

edited

Loading