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

Core tests: set up abstract class and remove code duplication #2295

Merged

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Dec 4, 2018

Note: this PR will be easiest to review by looking at the individual commits.

I've created this PR as a precursor to #2189 (move utility methods to static classes) as most of the new utility functions which are mentioned in that issue will be accompanied by unit tests when they are pulled.

Improving the setup of the utility function testing, as done in this PR, will make that so much simpler.


Commit Summary

New Tests\Core\AbstractMethodUnitTest class

Refactor to remove duplicate code.

This new class abstracts out the setUp() and tearDown() methods which were included in nearly all the utility method test classes.

The methods have been changed to static setUpBeforeClass() and tearDownAfterClass() with a static property as the initial processed File doesn't change between tests, so there is no need to tokenize and process the unit test case file for each test individually.

This makes the utility method unit tests significantly faster as well as lower the memory usage.

Additionally, the standard against which the files are processed has been changed from Generic (75 sniffs) to PSR1 (7 sniffs), which, again, makes the unit test suite for the Core utility functions significantly faster.

The current implementation allows for only one test case file per utility method test file, but as the utility method tests are not sniff specific, adding additional tests files for a method if needed, should not be a problem.

By default test case files are expected to be PHP inc files. Child classes can overload the static $fileExtension property when the test case file is a JS or CSS file.

Implement use of the new AbstractMethodUnitTest class

... in the various utility method unit test classes.

Tests\Core\AbstractMethodUnitTest: add new getTargetToken() method

This adds a new getTargetToken() method which can retrieve the target token for testing with higher precision than the (duplicate) code which was so far used in the individual test methods.

The improvements this method offers are:

  • Avoid test leaking/contamination.
    If/when the token to start the test from was retrieved by doing a findNext() from the delimiter comment, a typo could cause a token from the next test to be used for the testing instead of the target token.
    If the expected results would be the same for both tests, this would go completely unnoticed.
    This is now no longer possible.
    The only requirement is that the delimiter comments start with /* test....
  • No more token counting when setting up the unit tests, just pass the target token type constant to this method and it will get you the correct token.
    This also allows for not having to jump through hoops when deciding where to place the delimiter comment for the test.
  • If the token a test looks for is a T_STRING or text based token, the optional $tokenContent allows for selecting the correct token, even when there are several of the same type in the test case line.

Implement use of the new AbstractMethodUnitTest::getTargetToken() method

... in the various utility method unit test classes.

GetMethodParametersTest: remove even more duplicate code

GetMethodPropertiesTest: remove even more duplicate code

Implement use of the new AbstractMethodUnitTest::getTargetToken() method as well as abstract out the actual testing to a helper method as the logic was the same in all methods.

I've chosen not to implement this with a data provider as the separate test methods creating the $expected arrays make the tests more readable and easier to understand.

Tests\Core\AllTests: simplify creation of the test suite

As the refactor will add a lot of new unit test files, let's automate the creation of the test suite some more by automatically adding all Test files within the Test\Core directory.

@jrfnl jrfnl force-pushed the feature/core-tests-remove-code-duplication branch from 0b6c300 to 795297d Compare December 23, 2018 18:13
@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 23, 2018

Rebased for imaginary merge conflict.

@jrfnl jrfnl force-pushed the feature/core-tests-remove-code-duplication branch from 795297d to 2b0258e Compare January 8, 2019 03:04
@jrfnl jrfnl force-pushed the feature/core-tests-remove-code-duplication branch 3 times, most recently from 9515171 to f84d649 Compare February 19, 2019 19:04
@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 22, 2019

Could this PR be milestoned to 3.5.0 please ? It's a prerequisite for the utility method refactor.

@gsherwood gsherwood added this to the 3.5.0 milestone Feb 28, 2019
@jrfnl jrfnl force-pushed the feature/core-tests-remove-code-duplication branch from b4f3a4d to 2fda02c Compare February 28, 2019 19:09
@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 28, 2019

I've added an additional commit to this PR which simplifies the creation of the Core/utility method test suite as new tests don't need to be added manually anymore, but will automatically be included.

@jrfnl jrfnl force-pushed the feature/core-tests-remove-code-duplication branch 3 times, most recently from 05b5e35 to 19d9eb9 Compare March 4, 2019 18:45
@jrfnl jrfnl force-pushed the feature/core-tests-remove-code-duplication branch from 19d9eb9 to 7206279 Compare March 18, 2019 04:48
@jrfnl
Copy link
Contributor Author

jrfnl commented May 23, 2019

As discussed in #2189, this commit is included within #2456.

Refactor to remove duplicate code.

This new class abstracts out the `setUp()` and `tearDown()` methods which were included in nearly all the utility method test classes.

The methods have been changed to static `setUpBeforeClass()` and `tearDownAfterClass()` with a static property as the initial processed `File` doesn't change between tests, so there is no need to tokenize and process the unit test case file for each test individually.

This makes the utility method unit tests significantly faster as well as lower the memory usage.

Additionally, the _standard_ against which the files are processed has been changed from `Generic` (75 sniffs) to `PSR1` (7 sniffs), which again makes the unit test suite for the Core utility functions significantly faster.

The current implementation allows for only one test case file per utility method test file, but as the utility method tests are not sniff specific, adding additional tests files for a method if needed, should not be a problem.

By default test case files are expected to be PHP `inc` files. Child classes can overload the static `$fileExtension` property when the test case file is a JS or CSS file.
This adds a new `getTargetToken()` method which can retrieve the target token for testing with higher precision than the (duplicate) code which was so far used in the individual test methods.

The improvements this method offers are:
* Avoid test leaking/contamination.
    If/when the token to start the test from was retrieved by doing a `findNext()` from the delimiter comment, a typo could cause a token from the *next* test to be used for the testing instead of the target token.
    If the expected results would be the same for both tests, this would go completely unnoticed.
    This is now no longer possible.
    The only requirement is that the delimiter comments start with `/* test...`.
* No more token counting when setting up the unit tests, just pass the target token type constant to this method and it will get you the correct token.
    This also allows for not having to jump through hoops when deciding where to place the delimiter comment for the test.
* If the token a test looks for is a `T_STRING` or text based token, the optional `$tokenContent` allows for selecting the correct token, even when there are several of the same type in the test case line.
Implement use of the new AbstractMethodUnitTest::getTargetToken() method as well as abstract out the actual testing to a helper method as the logic was the same in all methods.

I've chosen not to implement this with a data provider as the separate test methods creating the `$expected` arrays make the tests more readable and easier to understand.

Also switched the actual assertion to `assertArraySubset()` which removes the need for all the `unset()`s for the exact token positions.
Ref: https://phpunit.de/manual/4.8/en/appendixes.assertions.html#appendixes.assertions.assertArraySubset
Implement use of the new AbstractMethodUnitTest::getTargetToken() method as well as abstract out the actual testing to a helper method as the logic was the same in all methods.

I've chosen not to implement this with a data provider as the separate test methods creating the `$expected` arrays make the tests more readable and easier to understand.

Also switched the actual assertion to `assertArraySubset()` which removes the need for all the `unset()`s for the exact token positions.
... from the `scripts` directory to the `tests` directory and namespace the class.
As the refactor will add a lot of new unit test files, let's automate the creation of the test suite some more by automatically adding all Test files within the `Test\Core` directory.
@jrfnl jrfnl force-pushed the feature/core-tests-remove-code-duplication branch from 7206279 to 3bb2ffa Compare August 27, 2019 04:46
@jrfnl
Copy link
Contributor Author

jrfnl commented Aug 27, 2019

I've rebased this PR, fixed the merge conflicts and added two additional commits and minor changes in the existing commits.

The significant differences with the original PR:

@gsherwood gsherwood merged commit 3bb2ffa into squizlabs:master Aug 29, 2019
@gsherwood
Copy link
Member

Thanks a lot for updating this PR.

The core test speed improvement was so noticeable that I had to go and break things just to make sure they were still running :)

@jrfnl jrfnl deleted the feature/core-tests-remove-code-duplication branch August 29, 2019 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants