Skip to content

Commit

Permalink
The unit test runner now loads the test sniff outside of the standard…
Browse files Browse the repository at this point in the history 
…'s ruleset so that exclude rules do not get applied (ref #1169)
  • Loading branch information
@gsherwood
gsherwood committed Sep 23, 2016
1 parent 7ede265 commit 3df85dc
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 14 deletions.
45 changes: 31 additions & 14 deletions CodeSniffer.php
View file
Expand Up @@ -533,34 +533,51 @@ public function initStandard($standards, array $restrictions=array(), array $exc
// be detected properly for files created on a Mac with the /r line ending.
ini_set('auto_detect_line_endings', true);

$sniffs = array();
foreach ($standards as $standard) {
$installed = $this->getInstalledStandardPath($standard);
if (defined('PHP_CODESNIFFER_IN_TESTS') === true) {
// Should be one standard and one sniff being tested at a time.
$installed = $this->getInstalledStandardPath($standards[0]);
if ($installed !== null) {
$standard = $installed;
} else {
$standard = self::realpath($standard);
$standard = self::realpath($standards[0]);
if (is_dir($standard) === true
&& is_file(self::realpath($standard.DIRECTORY_SEPARATOR.'ruleset.xml')) === true
) {
$standard = self::realpath($standard.DIRECTORY_SEPARATOR.'ruleset.xml');
}
}

if (PHP_CODESNIFFER_VERBOSITY === 1) {
$ruleset = simplexml_load_string(file_get_contents($standard));
if ($ruleset !== false) {
$standardName = (string) $ruleset['name'];
$sniffs = $this->_expandRulesetReference($restrictions[0], dirname($standard));

This comment has been minimized.

Sign in to view

Copy link
@jrfnl

jrfnl Sep 23, 2016

Contributor

Uh oh... this will break when the default values are used with an Undefined offset 0 notice.

This comment has been minimized.

Sign in to view

Copy link
@jrfnl

jrfnl Sep 23, 2016

Contributor

Changing line 536 from if (defined('PHP_CODESNIFFER_IN_TESTS') === true) to if (defined('PHP_CODESNIFFER_IN_TESTS') === true && empty($restrictions) === false) is a quick & dirty fix for this.

This comment has been minimized.

Sign in to view

Copy link
@gsherwood

gsherwood Sep 25, 2016

Author Member

@jrfnl Sorry, I made this assumption here because I didn't think any unit tests would run without specifying a single sniff. The problem this fixes is due to the fact that an entire standard is included, which isn't how the tests should have been run.

It's an easy fix so I'll add it, but I'm keen to know what sort of tests you have that caused this problem. More out of interest, but also so I don't break more stuff in the future.

This comment has been minimized.

Sign in to view

Copy link
@jrfnl

jrfnl Sep 26, 2016

Contributor

Thanks @gsherwood ! It was the PHPCompatibility unit test framework which was breaking.
You're right a single sniff should probably be specified there as well and I may try to work that in, but I'd need to study both frameworks in a lot more detail to see if I can. This seemed like a quick fix to get things working again.

The reason the PHPCompatibility project doesn't use the PHPCS native test framework is that for nearly all individual test cases they need to specify different command line config values (using PHP_CodeSniffer::setConfigData() in the test framework to do so). So they process each test case file with processFile() lots of times with different config values to test whether the sniffs work correctly.

The config values are needed to set which PHP versions the tested code should be compatibility with.
See https://github.com/wimg/PHPCompatibility#using-the-compatibility-sniffs for more info about the config values needed.
Hope that clarifies it.

This comment has been minimized.

Sign in to view

Copy link
@gsherwood

gsherwood Sep 26, 2016

Author Member

@jrfnl Thanks for that info.

I wonder if a combination of getTestFiles() and getCliValues() could be used to locate test files in the format SomethingUnitTest.[phpversion].php and then set the correct CLI args (using runtime-set to temp set config vars) for each file.

Both of those methods from AbstractSniffUnitTest were almost certainly written too recently to have been used originally, but it feels like there may be an alternate solution now. But that would be a complete rewrite of the way the custom testing framework, so it's more a thought experiment I think :)

This comment has been minimized.

Sign in to view

Copy link
@jrfnl

jrfnl Sep 26, 2016

Contributor

@gsherwood Thanks for your thoughts!
Will keep this in mind, but for now I don't think that will be possible as the sniff library also still supports PHPCS 1.x and that would mean the unit tests wouldn't be able to run for 1.x anymore.

The choice to still support PHPCS 1.x is a conscious one as the PHPCompatibility library is used when people either need to support various PHP versions or are upgrading software running on old PHP versions to newer ones. The "old" server where they initially want to run PHPCompatibility on might not be able to support the 2.x version.

Additionally, most tests need to be run against various versions (at least two) - one to check for the sniff showing a violation when support for a certain PHPversion (or range) is requested and one to check that the sniff does not show a violation when the requested PHPversion (or range) is higher (for new features) or lower (for old features).

Example: the sniff checking for new functions should throw an error if intdiv() if used in combination with a testVersion setting which includes 7.0, but if the requested support range does not include 7.0, it should show noViolation.

So while I like the file name idea, I don't think it will solve the issue of unit testing this sniff library (yet).

This comment has been minimized.

Sign in to view

Copy link
@gsherwood

gsherwood Sep 26, 2016

Author Member

@jrfnl Yep, no way to use those method on 1.x, although I didn't change the requirements between 1.x and 2.x (I don't think, it was a while ago now 😄 ) so it should work. That's not the same for 3.x though, but I did try and keep the required PHP version as low as possible there.

I do think the test file and CLI methods would be able to do what you're describing, as long as their is a solid naming convention for the files, but I'm not trying to push you in that direction and it would be a fair amount of work to try it out. I just wanted to see what sort of thing was being done to get ideas about additional flexibility for the test setup, and you've given me plenty of information to help there.

Thanks again.

This comment has been minimized.

Sign in to view

Copy link
@jrfnl

jrfnl Sep 26, 2016

Contributor

@gsherwood Nah, thank you for all the work you do on PHPCS. I cannot imagine the PHP infrastructure without it, so very much appreciate it.

Re - the filenames. when I have some time I'll try to look into this a little more. thanks for your thoughts on this.

This comment has been minimized.

Sign in to view

Copy link
@gsherwood

gsherwood Sep 26, 2016

Author Member

@jrfnl You're very welcome.
} else {
$sniffs = array();
foreach ($standards as $standard) {
$installed = $this->getInstalledStandardPath($standard);
if ($installed !== null) {
$standard = $installed;
} else {
$standard = self::realpath($standard);
if (is_dir($standard) === true
&& is_file(self::realpath($standard.DIRECTORY_SEPARATOR.'ruleset.xml')) === true
) {
$standard = self::realpath($standard.DIRECTORY_SEPARATOR.'ruleset.xml');
}
}

echo "Registering sniffs in the $standardName standard... ";
if (count($standards) > 1 || PHP_CODESNIFFER_VERBOSITY > 2) {
echo PHP_EOL;
if (PHP_CODESNIFFER_VERBOSITY === 1) {
$ruleset = simplexml_load_string(file_get_contents($standard));
if ($ruleset !== false) {
$standardName = (string) $ruleset['name'];
}

echo "Registering sniffs in the $standardName standard... ";
if (count($standards) > 1 || PHP_CODESNIFFER_VERBOSITY > 2) {
echo PHP_EOL;
}
}
}

$sniffs = array_merge($sniffs, $this->processRuleset($standard));
}//end foreach
$sniffs = array_merge($sniffs, $this->processRuleset($standard));
}//end foreach
}//end if

$sniffRestrictions = array();
foreach ($restrictions as $sniffCode) {
Expand Down
3 changes: 3 additions & 0 deletions package.xml
View file
Expand Up @@ -33,6 +33,9 @@ http://pear.php.net/dtd/package-2.0.xsd">
- Developers of custom standards with custom test runners can now have their standards ignored by the built-in test runner
-- Set the value of an environment variable called PHPCS_IGNORE_TESTS with a comma separated list of your standard names
-- Thanks to Juliette Reinders Folmer for the patch
- The unit test runner now loads the test sniff outside of the standard's ruleset so that exclude rules do not get applied
-- This may have caused problems when testing custom sniffs inside custom standards
-- Also makes the unit tests runs a little faster
- Fixed bug #1135 : PEAR.ControlStructures.MultiLineCondition.CloseBracketNewLine not detected if preceded by multiline function call
- Fixed bug #1138 : PEAR.ControlStructures.MultiLineCondition.Alignment not detected if closing brace is first token on line
- Fixed bug #1141 : Sniffs that check EOF newlines don't detect newlines properly when the last token is a doc block
Expand Down

0 comments on commit 3df85dc

Please sign in to comment.