Ruleset::populateTokenListeners(): add tests #757
Conversation
| $expected = ['PHP' => 'PHP']; | ||
|
|
||
| foreach (self::$ruleset->tokenListeners as $token => $listeners) { | ||
| $this->assertTrue(is_array($listeners), 'No listeners registered for token'.Tokens::tokenName($token)); |
There was a problem hiding this comment.
Was there a reason not to use sprintf here? Seems to be used in the other cases.
There was a problem hiding this comment.
There seem to be some other copies where this same construct is used, throughout this class.
There was a problem hiding this comment.
@NielsdeBlaauw Thanks for reviewing.
As for interpolation vs concatenation vs sprintf(): there is no hard rule being followed. The important thing here is that if a test contains more than one assertion, the $message parameter is being set for each assertion to make it more straight forward to know which assertion failed (if the test would fail). Especially if there are, for instance, three assertTrue() assertions, it becomes hard to distinguish which failed - "false is not true" - without those $message parameters.
For all practical purposes, whether interpolation vs concatenation vs sprintf() is used for the $message parameter is largely a balance of code readability, trying to keep the line length below 120 and trying not to violate the "concatenation operator is not allowed to have whitespace around it" CS rule which is being enforced for this codebase.
... and add one extra assertion to increase the stability of the tests.
There was a problem hiding this comment.
Thanks for this PR, @jrfnl. Overall, it looks good to me.
I left a few inline comments with some questions.
I also wonder why the test class for the Ruleset::populateTokenListeners() method tests the changes this method makes to the Ruleset::tokenListeners property, but not to other Ruleset public properties like Ruleset::sniffs and Rules::sniffCodes.
| * | ||
| * @return void | ||
| */ | ||
| public function testSetsSupportedTokenizersWhenProvidedBySniff($token) |
There was a problem hiding this comment.
Not directly related to this PR, and I imagine you already have this on your radar, but I wonder if we should add a check to Ruleset::populateTokenListeners() to ensure that $vars['supportedTokenizers'] is an array before calling foreach:
PHP_CodeSniffer/src/Ruleset.php
Line 1406 in 9c8f30f
There was a problem hiding this comment.
No, not really on my radar and as you correctly say: outside the scope of this PR. This PR is about adding tests for the existing code, not about improving the existing code.
Having said that, I'm not all that inclined to add an is_array() check for the following reasons:
- As of PHPCS 4.0, the property is no longer supported as there will only be one supported tokenizer.
- Guard code IMO should be mostly about protecting end users from accidental mistakes. Sniff developers should have at least a basic understanding of what they're doing.
- The property has always been an array property, there is no new or changed behaviour (other than the upcoming change in 4.0).
- If a sniff would do this wrong, I believe it is a good thing that this would lead to a PHP error during development of the sniff.
There was a problem hiding this comment.
Thanks for taking the time to elaborate on why you are not inclined to add an is_array() check. Makes sense to me.
Description
Suggested changelog entry
N/A
Related issues/external references
This PR is part of a series of PRs expanding the tests for the
Rulesetclass.