Fix failing unit tests / extended upstream classes #703

Merged
merged 6 commits into from Oct 5, 2016

Projects

None yet

3 participants

@jrfnl
Contributor
jrfnl commented Oct 5, 2016

Fixes #696

TL;DR: This PR fixes unnecessary dependencies on upstream sniffs and handles necessary dependencies more gracefully. Once this PR is merged, the unit tests will no longer fail.


There are four upstream sniffs which were being extended by one or more WP sniffs.
This could cause problems if something changes upstream and/or if the WPCS tests are not purely testing for WPCS additions, but also testing upstream checks - as was the case for the ArrayDeclaration sniff which was causing the unit tests to fail.

I've reviewed all relevant sniffs.

Generic.PHP.ForbiddenFunctions

There are four sniffs which extend this:

  • WordPress.PHP.DiscouragedFunctions
  • WordPress.VIP.FileSystemWritesDisallow
  • WordPress.VIP.SessionFunctionsUsage
  • WordPress.VIP.TimezoneChange

None of these duplicate the logic of the original class, they only provide the class with an array of functions to check for and in some cases, an adjusted error message.
The test files related to these sniffs only cover the functions provided by the child class.

Conclusion: usage as-is is safe.

Related #582, #633

PEAR.NamingConventions.ValidFunctionName

The WordPress.NamingConventions.ValidFunctionName sniff extends this.
In this case, the exact opposite applies: the functional logic is not used at all, only the class properties of the original class is used to prevent duplication of the maintenance of these (native PHP) exceptions to the rules.
The test files in this case do test that the properties from the parent are handled appropriately, so there is some bleed through.

As the properties we're talking about are a list of the PHP magic methods, I don't expect any to be removed any time soon, but we will automatically get the benefit of any being added upstream if PHP would add more magic methods.

Conclusion: Low risk, leave as is.

Squiz.WhiteSpace.CastSpacing

The WordPress.WhiteSpace.CastStructureSpacing sniff was based upon this sniff and extended it.
This was a recent change. Previously the logic of the parent was duplicated. This duplication was removed early in the 0.11.0 cycle in favour of extending the sniff.
The WP version adds two additional checks to the parent.
Upon further review, the parent does not need to be extended, but can be added to the ruleset as is. The two additional checks become a stand-alone sniff. The only code duplication involved is the register() method.
The tests did contain bleed through, testing both the parent as well as the child. This has been cleaned up.

Conclusion: This sniff should be - and has been - turned into a stand-alone sniff.

Squiz.Arrays.ArrayDeclaration

This one is the most awkward and complicated one and is extended by the WordPress.Arrays.ArrayDeclaration sniff.
The parent class does some initial processing which already contains three checks and then - depending on the kind of array - passes off to either the processSingleLineArray() or the processMultiLineArray() method.
The logic of the processSingleLineArray() method of the parent is used as-is, one check contained there-in is excluded via the ruleset and four additional checks are added in the child.
The logic of the processMultiLineArray() method is however duplicated in the child with some code removed. This is/was needed as the parent returns early for some checks which we want to exclude. However the excluding from the ruleset does not prevent the return from being enacted, which would have the unintended side-effect of later checks being ignored.

This was already reported upstream in squizlabs/PHP_CodeSniffer#582 by @JDGrimes over a year ago and while the intention (upstream) is to split this sniff into more re-usable parts which will live in the Generic standard, this is a dormant ticket.

The problem here is that this is the only upstream sniff dealing with array token walking, that the logic is sound and that we want/need 70% of the checks being done by the sniff.

The test file contains a lot* of bleed through though - 99% of all tests relate to the upstream checks.

My take after investigating was as follows:

  • Create a new WordPress.Arrays.ArrayDeclarationSpacing sniff for the additional checks in processSingleLineArray(). This sniff will extend the parent, but only for the process logic to get to the processSingleLineArray() method. The checks in the initial processing will be excluded from the ruleset to prevent duplicate messages being thrown.
    The tests for the new sniff will only test the WP specific additional checks without bleed through.
  • For the generic checks in the initial parent process() and select checks in the processMultiLineArray(), the WordPress.Arrays.ArrayDeclaration remains.
    The class will still extend the parent and the processMultiLineArray() will still be duplicated with a select number of checks commented out.
    As the class now doesn't add anything to the parent - just removes checks - no separate test file is needed as all checks are already being tested upstream.
    As this sniff will from time to time need to be synced with upstream to keep up with fixes made there, I've largely reverted the WPCS code style changes for this file and excluded this file from being sniffed for code style. This will facilitate much easier syncing with upstream.

Conclusion: To allow for a) testing our own additions in isolation and b) still getting the benefit of the upstream sniff, the sniff needs to be - and has been - split into two separate sniffs which both will still extend the upstream sniff.

Other notes

Where necessary I've added additional documentation about this for anyone looking at these sniffs in the future.


Ok, so this has become quite a long tale, but it seemed prudent to document this carefully and in detail.

For anyone who's read this far: kudo's & let's merge this baby ;-)

jrfnl added some commits Oct 4, 2016
@jrfnl jrfnl Add internal note to ValidFunctionNameSniff about why this extends an…
… upstream sniff.
cd30414
@jrfnl jrfnl Uncouple the CastStructureSpacingSniff from the upstream parent.
The parent is used as is and uncoupling hardly leads to duplicate code (only the `register` method).

* Updated the related ruleset.
* Updated the documentation to reflect the change.
* Updated the unit tests to only deal with the checks within "our" sniff.
* Added documentation to the unit test cases.
e69dcdd
@jrfnl jrfnl Move the WP specific additional array sniffs to a separate sniff file.
Also: fix the error codes which were incorrectly named.
13f19c9
@jrfnl jrfnl Revert code style changes and removal of commented out code to facili…
…tate easier sync compare.

Exclude file from WPCS codestyle check.
afed022
@jrfnl jrfnl Sync in upstream fixes to the `ArrayDeclaration` sniff. 2ef158d
@jrfnl jrfnl Document the changes to the `ArrayDeclaration` sniff and remove the t…
…est files.
683cf57
@jrfnl jrfnl added this to the 0.11.0 milestone Oct 5, 2016
@jrfnl jrfnl changed the title from Feature/split off wp sniffs to Fix failing unit tests / extended upstream classes Oct 5, 2016
@jrfnl jrfnl referenced this pull request Oct 5, 2016
Closed

Don't extend upstream sniffs. #696

+ $phpcsFile->fixer->replaceToken( ( $arrayEnd - 1 ), str_repeat(' ', $expected ) );
+ }
+ }
+ */
@westonruter
westonruter Oct 5, 2016 Member

Can this commented-out code be removed?

@jrfnl
jrfnl Oct 5, 2016 edited Contributor

I put the commented out code back in on purpose so as to match the original code as closely as possible and make both future syncing compares with the original class as well as understanding why this class needs to be an extended class easier to understand.

As the method is now a duplicate of the original with the only real difference the comment marks, this will save a maintainer in the future probably a couple of hours.

+ $error = 'No key specified for array entry; first entry specifies key';
+ $phpcsFile->addError( $error, $nextToken, 'NoKeySpecified' );
+ return;
+ }
@westonruter
westonruter Oct 5, 2016 Member

Can this commented-out code be removed?

@jrfnl
jrfnl Oct 5, 2016 Contributor

See above

@@ -221,7 +187,7 @@ public function processMultiLineArray( PHP_CodeSniffer_File $phpcsFile, $stackPt
);
$fix = $phpcsFile->addFixableError( $error, $nextToken, 'SpaceBeforeComma', $data );
- if ( true === $fix ) {
+ if ($fix === true) {
@westonruter
westonruter Oct 5, 2016 Member

Why the removal of the whitespace and the yoda conditions?

@jrfnl
jrfnl Oct 5, 2016 Contributor

Same reason.

+ $error = 'Key specified for array entry; first entry has no key';
+ $phpcsFile->addError( $error, $nextToken, 'KeySpecified' );
+ return;
+ }
@westonruter
westonruter Oct 5, 2016 Member

Can this commented-out code be removed?

@jrfnl
jrfnl Oct 5, 2016 Contributor

See above

+
+ return;
+ } // end if
+ */
@westonruter
westonruter Oct 5, 2016 Member

Shouldn't this commented-out code be removed?

@jrfnl
jrfnl Oct 5, 2016 Contributor

See above

+ }
+ }
+ }
+ */
@westonruter
westonruter Oct 5, 2016 Member

Shouldn't this commented-out code just be removed?

@jrfnl
jrfnl Oct 5, 2016 Contributor

dito

+ }
+ }
+ } // end if
+ */
@westonruter
westonruter Oct 5, 2016 Member

Should this commented-out code just be removed?

@jrfnl
jrfnl Oct 5, 2016 Contributor

dito

// Check each line ends in a comma.
$valueLine = $tokens[ $index['value'] ]['line'];
$nextComma = false;
for ( $i = $index['value']; $i < $arrayEnd; $i++ ) {
// Skip bracketed statements, like function calls.
- if ( T_OPEN_PARENTHESIS === $tokens[ $i ]['code'] ) {
+ if ($tokens[$i]['code'] === T_OPEN_PARENTHESIS) {
@westonruter
westonruter Oct 5, 2016 Member

Why the de-yodification?

@jrfnl
jrfnl Oct 5, 2016 Contributor

See above

@westonruter westonruter merged commit 778588d into develop Oct 5, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@westonruter westonruter deleted the feature/split-off-WP-sniffs branch Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment