Tests: stability tweaks for the non-sniff tests #576
Merged
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
…ties ... to prevent changes to the values of these for one test, influencing another test. This is only a stability tweak, there are no existing tests affected by this issue at this time.
The `Config` class uses a number of static properties, which may be updated during tests. These were previously - prior to 275 - reset to their default values in the `AbstractMethodUnitTest::resetTestFile()` method, but this reset was inadvertently removed 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 `AbstractMethodUnitTest::reset()` 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.
This is only a stability tweak, there are no existing tests affected by this issue at this time.
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.
Description
AbstractMethodUnitTest: add "tear down" method to reset static properties
... to prevent changes to the values of these for one test, influencing another test.
This is only a stability tweak, there are no existing tests affected by this issue at this time.
Tests/ConfigDouble: bug fix - always reset Config statics after use
The
Configclass uses a number of static properties, which may be updated during tests. These were previously - prior to #275 - reset to their default values in theAbstractMethodUnitTest::resetTestFile()method, but this reset was inadvertently removed 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 theAbstractMethodUnitTest::reset()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.This is only a stability tweak, there are no existing tests affected by this issue at this time.
Suggested changelog entry
N/A