Skip to content

Tests/Tokenizer: expand default keyword tests #813

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 22, 2025
Merged

Tests/Tokenizer: expand default keyword tests #813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions tests/Core/Tokenizers/Tokenizer/RecurseScopeMapDefaultKeywordConditionsTest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,29 @@ function switchWithDefaultInMatch() {
};
}

function switchAndDefaultSharingScopeCloser($i) {
switch ($i):
/* testSwitchAndDefaultSharingScopeCloser */
default:
echo 'one';
endswitch;
}

function switchDefaultNestedIfWithAndWithoutBraces($i, $foo, $baz) {
switch ($i) {
/* testSwitchDefaultNestedIfWithAndWithoutBraces */
default:
if ($foo) {
return true;
} elseif ($baz)
return true;
else {
echo 'else';
}
break;
}
}

function shortArrayWithConstantKey() {
$arr = [
/* testClassConstantAsShortArrayKey */
Expand Down
59 changes: 40 additions & 19 deletions tests/Core/Tokenizers/Tokenizer/RecurseScopeMapDefaultKeywordConditionsTest.php
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the other PR:

Should this improvement also be done RecurseScopeMapDefaultKeywordConditionsTest tests ?

it seems to me that adding the closer markers to RecurseScopeMapDefaultKeywordConditionsTest is not necessary as the tests there are different. The code doesn't look for the scope_closer using AbstractTokenizerTestCase::getTargetToken() and the test marker. Instead, a closerOffset is passed, and it is used to determine the scope_closer:

I think that was the whole point of the original remark: having a marker for the closer will make the tests more stable, than manually setting closerOffset expectations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I meant: 82fae8e

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrfnl, thanks for clarifying and for providing an example. Besides the improvements from the commit that you mentioned, I believe I have seen the name of the marker included in the $message parameter passed to the assertion methods in another test file. Something like:

$this->assertArrayHasKey(
    'scope_condition',
    $tokenArray,
    sprintf('Scope condition is not set. Marker: %s.', $testMarker)
);

I like this pattern as it makes it easier to identify the related test case when there is a failure. Since I will improve RecurseScopeMapDefaultKeywordConditionsTest::testSwitchDefault() in this PR, should I add the marker to the error message?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodrigoprimo Sounds like a good idea 👍🏻

Original file line number Diff line number Diff line change
Expand Up @@ -109,19 +109,23 @@ public static function dataMatchDefault()
* Note: Cases and default structures within a switch control structure *do* get case/default scope
* conditions.
*
* @param string $testMarker The comment prefacing the target token.
* @param int $openerOffset The expected offset of the scope opener in relation to the testMarker.
* @param int $closerOffset The expected offset of the scope closer in relation to the testMarker.
* @param int|null $conditionStop The expected offset in relation to the testMarker, at which tokens stop
* having T_DEFAULT as a scope condition.
* @param string $testContent The token content to look for.
* @param string $testMarker The comment prefacing the target token.
* @param int $openerOffset The expected offset of the scope opener in relation to the testMarker.
* @param int $closerOffset The expected offset of the scope closer in relation to the testMarker.
* @param int|null $conditionStop The expected offset in relation to the testMarker, at which tokens stop
* having T_DEFAULT as a scope condition.
* @param string $testContent The token content to look for.
* @param bool $sharedScopeCloser Whether to skip checking for the `scope_condition` of the
* scope closer. Needed when the default and switch
* structures share a scope closer. See
* https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/810.
*
* @dataProvider dataSwitchDefault
* @covers PHP_CodeSniffer\Tokenizers\Tokenizer::recurseScopeMap
*
* @return void
*/
public function testSwitchDefault($testMarker, $openerOffset, $closerOffset, $conditionStop=null, $testContent='default')
public function testSwitchDefault($testMarker, $openerOffset, $closerOffset, $conditionStop=null, $testContent='default', $sharedScopeCloser=false)
{
$tokens = $this->phpcsFile->getTokens();

Expand All @@ -148,13 +152,17 @@ public function testSwitchDefault($testMarker, $openerOffset, $closerOffset, $co
$this->assertSame($expectedScopeOpener, $tokens[$opener]['scope_opener'], 'T_DEFAULT opener scope opener token incorrect');
$this->assertSame($expectedScopeCloser, $tokens[$opener]['scope_closer'], 'T_DEFAULT opener scope closer token incorrect');

$closer = $tokenArray['scope_closer'];
$this->assertArrayHasKey('scope_condition', $tokens[$closer], 'Closer scope condition is not set');
$this->assertArrayHasKey('scope_opener', $tokens[$closer], 'Closer scope opener is not set');
$this->assertArrayHasKey('scope_closer', $tokens[$closer], 'Closer scope closer is not set');
$this->assertSame($token, $tokens[$closer]['scope_condition'], 'Closer scope condition is not the T_DEFAULT token');
$this->assertSame($expectedScopeOpener, $tokens[$closer]['scope_opener'], 'T_DEFAULT closer scope opener token incorrect');
$this->assertSame($expectedScopeCloser, $tokens[$closer]['scope_closer'], 'T_DEFAULT closer scope closer token incorrect');
$closer = $expectedScopeCloser;

if ($sharedScopeCloser === false) {
$closer = $tokenArray['scope_closer'];
$this->assertArrayHasKey('scope_condition', $tokens[$closer], 'Closer scope condition is not set');
$this->assertArrayHasKey('scope_opener', $tokens[$closer], 'Closer scope opener is not set');
$this->assertArrayHasKey('scope_closer', $tokens[$closer], 'Closer scope closer is not set');
$this->assertSame($token, $tokens[$closer]['scope_condition'], 'Closer scope condition is not the T_DEFAULT token');
$this->assertSame($expectedScopeOpener, $tokens[$closer]['scope_opener'], 'T_DEFAULT closer scope opener token incorrect');
$this->assertSame($expectedScopeCloser, $tokens[$closer]['scope_closer'], 'T_DEFAULT closer scope closer token incorrect');
}

if (($opener + 1) !== $closer) {
$end = $closer;
Expand Down Expand Up @@ -184,12 +192,12 @@ public function testSwitchDefault($testMarker, $openerOffset, $closerOffset, $co
public static function dataSwitchDefault()
{
return [
'simple_switch_default' => [
'simple_switch_default' => [
'testMarker' => '/* testSimpleSwitchDefault */',
'openerOffset' => 1,
'closerOffset' => 4,
],
'simple_switch_default_with_curlies' => [
'simple_switch_default_with_curlies' => [
// For a default structure with curly braces, the scope opener
// will be the open curly and the closer the close curly.
// However, scope conditions will not be set for open to close,
Expand All @@ -199,21 +207,34 @@ public static function dataSwitchDefault()
'closerOffset' => 12,
'conditionStop' => 6,
],
'switch_default_toplevel' => [
'switch_default_toplevel' => [
'testMarker' => '/* testSwitchDefault */',
'openerOffset' => 1,
'closerOffset' => 43,
],
'switch_default_nested_in_match_case' => [
'switch_default_nested_in_match_case' => [
'testMarker' => '/* testSwitchDefaultNestedInMatchCase */',
'openerOffset' => 1,
'closerOffset' => 20,
],
'switch_default_nested_in_match_default' => [
'switch_default_nested_in_match_default' => [
'testMarker' => '/* testSwitchDefaultNestedInMatchDefault */',
'openerOffset' => 1,
'closerOffset' => 18,
],
'switch_and_default_sharing_scope_closer' => [
'testMarker' => '/* testSwitchAndDefaultSharingScopeCloser */',
'openerOffset' => 1,
'closerOffset' => 10,
'conditionStop' => null,
'testContent' => 'default',
'sharedScopeCloser' => true,
],
'switch_and_default_with_nested_if_with_and_without_braces' => [
'testMarker' => '/* testSwitchDefaultNestedIfWithAndWithoutBraces */',
'openerOffset' => 1,
'closerOffset' => 48,
],
];

}//end dataSwitchDefault()
Expand Down