Skip to content
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

Jump to bottom

[FrameworkBundle] KernelTestCase resets internal state on tearDown #45414

Merged
merged 1 commit into from Feb 18, 2022
Merged

[FrameworkBundle] KernelTestCase resets internal state on tearDown #45414

merged 1 commit into from Feb 18, 2022
+19 −0

Conversation

core23
Copy link
Contributor

@core23 core23 commented Feb 13, 2022 •
edited by nicolas-grekas

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

When using the KernelTestCase for multiple different test kernels, the KernelTestCase::$class is not reseted after the test has finished. All other class variables are set to the initial state, so this change should be a bugfix.

@carsonbot carsonbot added this to the 6.1 milestone Feb 13, 2022
@carsonbot carsonbot changed the title KernelTestCase resets internal state on tearDown [FrameworkBundle] KernelTestCase resets internal state on tearDown Feb 13, 2022
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 13, 2022

Did you experience any nasty side effect of this not being reset? Can you add a corresponding test case?

@nicolas-grekas nicolas-grekas removed this from the 6.1 milestone Feb 13, 2022
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Feb 13, 2022
@core23 core23 force-pushed the reset-kernel-test branch from 53c0f9f to 1f205d8 Feb 13, 2022
@core23
Copy link
Contributor Author

@core23 core23 commented Feb 13, 2022

Did you experience any nasty side effect of this not being reset?

IMHO it should only have a minimum performance impact, because the name is resolved for every test:

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

Lines 126 to 128 in 53c0f9f

if (null === static::$class) {
static::$class = static::getKernelClass();
}

@core23 core23 force-pushed the reset-kernel-test branch from 1f205d8 to 3debd21 Feb 13, 2022
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 14, 2022

It could have a performance impact. That's why I'm asking. Why do you want to reset this property, if everything is fine when it is not reset?

@core23
Copy link
Contributor Author

@core23 core23 commented Feb 14, 2022

I have some projects that will perform web tests with different kernel configuration. This can also be the case, if you provide a bundle with optional dependencies that will result in a slightly different service configuration.

At the moment, I have to manuell reset the ::$class after every test with a local tearDown method. When I don't do this, the first (randomly selected) test will reserve the kernel class and all other test will fail, because they expect a different kernel class.

nicolas-grekas
nicolas-grekas approved these changes Feb 18, 2022
View changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Thanks for the explanation, that's what I was missing.

@nicolas-grekas nicolas-grekas changed the base branch from 6.1 to 4.4 Feb 18, 2022
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 18, 2022

Thank you @core23.

@nicolas-grekas nicolas-grekas merged commit 3e9a6e3 into symfony:4.4 Feb 18, 2022
9 of 10 checks passed
@core23 core23 deleted the reset-kernel-test branch Feb 18, 2022
@fabpot fabpot mentioned this pull request Feb 28, 2022
Merged
@fabpot fabpot mentioned this pull request Feb 28, 2022
Merged
@fabpot fabpot mentioned this pull request Feb 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas

@lyrixx lyrixx

@yceruto yceruto

@wouterj wouterj

@chalasr chalasr

@dunglas dunglas

@xabbuh xabbuh

Assignees
No one assigned
Labels
Bug FrameworkBundle Status: Needs Review
Projects
None yet
Milestone
4.4
Linked issues

Successfully merging this pull request may close these issues.

None yet
3 participants
@core23 @nicolas-grekas @carsonbot