- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
PhpUnitExpectationFixer - update for Phpunit 8.4 #4908
PhpUnitExpectationFixer - update for Phpunit 8.4 #4908
Conversation
f142eba
to
ae02f94
Compare
a couple of phpunit assertions are deprecated in phpunit 8 and the test-suite was very mixed for exceptions (annotations, set... and expect...). heavy lifting of with PHP-CS-Fixer: // assertInternalType(<type> >>> assertIs<type>( 'php_unit_dedicate_assert_internal_type' => true, // @ExpectedException... >>> ->setExpectedException(... 'php_unit_no_expectation_annotation' => ['use_class_const' => false], // ->setExpectedException...( >>> ->expectException...( 'php_unit_expectation' => true, via scripts/csfix.sh: usage examples: $ PHP_CS_FIXER_BIN=../PHP-CS-Fixer/php-cs-fixer unbuffer lib/scripts/csfix.sh \ --dry-run --diff -- tests/unit tests/integration | less -R $ PHP_CS_FIXER_BIN=../PHP-CS-Fixer/php-cs-fixer lib/scripts/csfix.sh \ -- tests/unit tests/integration some places aren't caught thought and some aren't "fixed", so here are some details: - YamlTester does not extend a test-case, this is why PHP-CS-Fixer can not detect it (?). - assertContains for string to assertStringContainsString as PHP-CS-Fixer has no fixer for it (yet as phpunit 8, little use here) - move away from setExpectedException to expectException, using a patched PHP-CS-Fixer (PHP-CS-Fixer/PHP-CS-Fixer#4908)
a couple of phpunit assertions are deprecated in phpunit 8 and the test-suite was very mixed for exceptions (annotations, set... and expect...). heavy lifting of with PHP-CS-Fixer: // assertInternalType(<type> >>> assertIs<type>( 'php_unit_dedicate_assert_internal_type' => true, // @ExpectedException... >>> ->setExpectedException(... 'php_unit_no_expectation_annotation' => ['use_class_const' => false], // ->setExpectedException...( >>> ->expectException...( 'php_unit_expectation' => true, via scripts/csfix.sh: usage examples: $ PHP_CS_FIXER_BIN=../PHP-CS-Fixer/php-cs-fixer unbuffer lib/scripts/csfix.sh \ --dry-run --diff -- tests/unit tests/integration | less -R $ PHP_CS_FIXER_BIN=../PHP-CS-Fixer/php-cs-fixer lib/scripts/csfix.sh \ -- tests/unit tests/integration some places aren't caught thought and some aren't "fixed", so here are some details: - YamlTester does not extend a test-case, this is why PHP-CS-Fixer can not detect it (?). - assertContains for string to assertStringContainsString as PHP-CS-Fixer has no fixer for it (yet as phpunit 8, little use here) - move away from setExpectedException to expectException, using a patched PHP-CS-Fixer (PHP-CS-Fixer/PHP-CS-Fixer#4908)
ae02f94
to
ef279df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add @PHPUnit84Migration:risky
in RuleSet
?
This is a new feature, can you rebase onto master please? |
Can't we also map |
ef279df
to
ae3ff0d
Compare
ruleset was missing. //cc @kubawerlos Ref: PHP-CS-Fixer#4908
and:
Does adding a new rule-set in this case count as new feature? Just wondering, as for this PR this is merely a fix to use the fixer for phpunit migration towards a PHP 7.4 target (coming from PHP 7.3 with backwards compat PHP 5.3 baseline in the project). For the change (not part of this PR and I could also benefit of help with using standard methods in renaming method calls) to upgrade the previous deprecated method to phpunit 8.4 new ones, this would be a different fixer (and I would be happy to base it onto master, no problem with that). Were you concerned about that part? Never edited RuleSet, so also please bear with me having such kind of questions. |
Yes (it changes the public API of the tool), and so does adding a new value to an existing option of a rule.
IMO it would be better to implement it in the same fixer. But I'm not familiar with this one so I don't know whether it's easily feasable. |
@julienfalque I considered the fix here an easy catch - until I now changed the RuleSet and the tests fail and I don't understand what the test is testing when it fails. So better keep things smaller for this one and do it right. If someone could take a look at the failing test for the fixer configuration and could give me some pointers would be great. |
The failures are because:
|
@julienfalque @ktomk also some other tests for rule sets are failing. |
- Wrong rule used in rule-set - Skip new @PHPUnit84Migration:risky rule in integration tests Ref: PHP-CS-Fixer#4908
32da260
to
4936aaa
Compare
ruleset was missing. //cc @kubawerlos Ref: PHP-CS-Fixer#4908
- Wrong rule used in rule-set - Skip new @PHPUnit84Migration:risky rule in integration tests Ref: PHP-CS-Fixer#4908
@ktomk Looking good. Could add or update a test showing that an existing call to |
Ongoing with Phpunit 8.4 the expectExceptionMessageRegExp() method is deprecated in favor of the expectExceptionMessageMatches() method. This change set patches the rename of expectExceptionMessageMatches to expectExceptionMessageMatches into the expectation fixer. NOTE: If the original method call in the code has a different argument count than one, the behavior is undefined. Ref: PHP-CS-Fixer#4908
Actually this fixer did not come with such a fix which I noted early on, and that I was under the impression to have it a fixer of it's own (like on a base rename method fixer or similar). However your last comment made me curious so I wrote a test for exactly this and patched the fixer. It's kinda patched in, so this needs review. |
Ongoing with Phpunit 8.4 the expectExceptionMessageRegExp() method is deprecated in favor of the expectExceptionMessageMatches() method. This change set patches the rename of expectExceptionMessageMatchesRegExp to expectExceptionMessageMatches into the expectation fixer. NOTE: If the original method call in the code has a different argument count than one, the behavior is undefined. Ref: PHP-CS-Fixer#4908
ba65ed1
to
0bab66d
Compare
Ongoing with Phpunit 8.4 the expectExceptionMessageRegExp() method is deprecated in favor of the expectExceptionMessageMatches() method. This change set patches the rename of expectExceptionMessageMatchesRegExp to expectExceptionMessageMatches into the expectation fixer. NOTE: If the original method call in the code has a different argument count than one, the behavior is undefined. Ref: PHP-CS-Fixer#4908
0bab66d
to
debee43
Compare
…etVersion has its set in RuleSet (kubawerlos) This PR was merged into the 2.15 branch. Discussion ---------- DX: add test to ensure each target version in PhpUnitTargetVersion has its set in RuleSet Would have help getting feedback faster in #4908. Commits ------- 968d095 DX: add test to ensure each target version in PhpUnitTargetVersion has its set in RuleSet
Ongoing with Phpunit 8.4 the expectExceptionMessageRegExp() method is deprecated in favor of the expectExceptionMessageMatches() method. This change set patches the rename of expectExceptionMessageMatchesRegExp to expectExceptionMessageMatches into the expectation fixer. NOTE: If the original method call in the code has a different argument count than one, the behavior is undefined. Ref: PHP-CS-Fixer#4908
debee43
to
ff08f1f
Compare
Ongoing with Phpunit 8.4 the expectExceptionMessageRegExp() method is deprecated in favor of the expectExceptionMessageMatches() method. This change set patches the rename of expectExceptionMessageMatchesRegExp to expectExceptionMessageMatches into the expectation fixer. NOTE: If the original method call in the code has a different argument count than one, the behavior is undefined. Ref: PHP-CS-Fixer#4908
ff08f1f
to
65e63b5
Compare
Hi there, just refreshing again as it starts to rot a bit here. Are there any blockers? |
65e63b5
to
ced5045
Compare
Thank you @ktomk. |
With Phpunit 8.4 the method expectExceptionMessageRegExp has been
deprecated and it's replacement is the new expectExceptionMessageMatches.
When running the migration to target a more current Phpunit version, the
php_unit_expectation fixer previously did choose the deprecated (old)
method.
Improvement here is to map to the non-deprecated method for Phpunit 8.4+.
NOTE: The old method is removed in Phpunit 9 (not 10!)
Additionally the PhpUnitExpectationFixer fixer is extended replacing the deprecated method expectExceptionMessageRegExp with expectExceptionMessageMatches.
Ref: sebastianbergmann/phpunit@d1199cb