TestUtils/ConfigDouble: bug fix - always reset Config statics after use #612
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The PHPCS native `Config` class uses a number of static properties, which may be updated during tests. These were previously reset to their default values in the `UtilityMethodTestCase::resetTestFile()` method, but this reset was inadvertently removed in commit 4f0f9a4 with the reasoning that, if all tests use the `ConfigDouble`, the reset would no longer be needed as the `ConfigDouble` resets on being initialized. The flaw in this logic is that not all tests are guaranteed to use the `ConfigDouble`, which means that without the reset, the `Config` class may be left "dirty" after tests using the `ConfigDouble`, which could break tests. This commit fixes this issue by: * Adding a `__destruct()` method to the `ConfigDouble` class which will reset the static properties on the PHPCS native `Config` class whenever an object created from this class is destroyed. * Explicitly calling the `__destruct()` method from the `UtilityMethodTestCase::resetTestFile()` method to ensure it is always run after a test has finished ("after class"), even if there would still be a lingering reference to the object. Includes tests for the new `__destruct()` method. Includes an additional test for the `UtilityMethodTestCase` class to ensure this snafu cannot come back without tests failing on it.
499e04a to
acd940a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PHPCS native
Configclass uses a number of static properties, which may be updated during tests. These were previously reset to their default values in theUtilityMethodTestCase::resetTestFile()method, but this reset was inadvertently removed in commit 4f0f9a4 with the reasoning that, if all tests use theConfigDouble, the reset would no longer be needed as theConfigDoubleresets on being initialized.The flaw in this logic is that not all tests are guaranteed to use the
ConfigDouble, which means that without the reset, theConfigclass may be left "dirty" after tests using theConfigDouble, which could break tests.This commit fixes this issue by:
__destruct()method to theConfigDoubleclass which will reset the static properties on the PHPCS nativeConfigclass whenever an object created from this class is destroyed.__destruct()method from theUtilityMethodTestCase::resetTestFile()method to ensure it is always run after a test has finished ("after class"), even if there would still be a lingering reference to the object.Includes tests for the new
__destruct()method.Includes an additional test for the
UtilityMethodTestCaseclass to ensure this snafu cannot come back without tests failing on it.Props to @fredden for helping me debug this issue.