Hey!

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

Symfony has acontribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (seehttps://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://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

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

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

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

@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 theHttpClientTestCase 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 markingensureKernelShutdown as@afterClass

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

Thank you@cquintana92.

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.

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

Reverted in0291ea1

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

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

@nicolas-grekas there you go:#60693