Tests/Tokenizer: expand default keyword tests

[rodrigoprimo]

Description

This PR adds two new tests to the tokenizer tests for the default keyword:

• Ensure that the default scope is set correctly when the scope closer is shared with a switch.
• Ensure that the default scope is set correctly when there is an inner if condition with and without braces.

Both tests were copied from similar tests for the case keyword that were added to protect against regressions (see b9809c7 and 61b9e9b). They were suggested in #595 (comment) and #595 (comment).

Related issues/external references

Tests suggested in #595 (comment) and #595 (comment).

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

The test has braces for all if/else leaves, while the point of the test was that it would not (also see the marker name)....

        if ($foo) {
            return true;
        } elseif ($baz)
            return true;
        else {
            echo 'else';
        }

[rodrigoprimo]

Thanks for catching this. It is now fixed in f0e74de

[jrfnl]

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.

[jrfnl]

This is what I meant: 82fae8e

[rodrigoprimo]

@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?

[jrfnl]

@rodrigoprimo Sounds like a good idea 👍🏻

[rodrigoprimo]

I marked this PR as "blocked" and created a separate PR to address a bug in the default keyword tests that I found: #850. Both PRs touch the exact same part of the code. So to avoid conflicts, I will wait for a decision about the other PR before asking for another review on this one.

[rodrigoprimo]

PR #850 was merged, but while rebasing this PR I noticed something that I missed and created another PR to address it: #870

[rodrigoprimo]

@jrfnl, now that both PRs blocking this one have been merged, I have rebased it, fixed a merge conflict and now it is ready for another review. Thanks.

[rodrigoprimo]

The label and review requests changes above were done to test #897 and #898.

[jrfnl]

Eh ... no. That's not how a multi-line function signature should look.

Moreover, I don't see a reason for a multi-line signature in the first place. We know we are in the tests, so the parameters don't need to tell us.

Suggested change

	public function testSwitchDefault(
	$testMarker, $testOpenerMarker=null, $testCloserMarker=null, $conditionStopMarker=null, $testContent='default', $sharedScopeCloser=false
	) {
	public function testSwitchDefault($testMarker, $openerMarker=null, $closerMarker=null, $conditionStopMarker=null, $testContent='default', $sharedScopeCloser=false) {

Also, not so sure about the null default values for the changed parameters. It doesn't make things clearer.

[jrfnl]

We've discussed this PR in a call. The fourth commit is definitely not acceptable as-is (for various reasons).

The first commit (when combined with the second + fifth) is okay, though I'm in two minds about the new parameter for shared closers as it makes the test inconsistent with the sister-test for the case keyword.

The third commit would be better off as a separate PR.

All in all, I don't think this is ready for merge.

[rodrigoprimo]

Thanks for checking this PR, @jrfnl.

I made the changes we discussed, combining commits 1, 2, and 5 into one and dropping commits 3 and 4. Could you please take another look?

I will create another PR with commit 3 once this one is merged and then another PR for commit 4 after improving it.

[jrfnl]

Thanks @rodrigoprimo ! This set of changes I can accept.