Closed (fixed)
Project:
Drupal core
Version:
8.8.x-dev
Component:
phpunit
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
3 Jun 2019 at 11:47 UTC
Updated:
25 Jun 2019 at 10:59 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
mondrakeIn #3053417: Clean up PhpunitCompatibilityTrait it was discussed to deprecate
\Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException().Do that, and replace calls with
$this->expectExceptionand$this->expectExceptionMessage.Comment #3
mondrakeComment #4
sershevchykComment #5
christinlepson commentedDeprecated the
\Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException()function and replaced a total of 517 calls.197 single parameter calls were replaced with
$this->expectExceptionand 320 calls replaced with$this->expectExceptionand$this->expectExceptionMessage.Comment #6
christinlepson commentedComment #7
christinlepson commentedDrupal\Tests\jsonapi\Kernel\Context\FieldResolverTestneeded anifstatement to call$this->expectExceptionMessageonly when the message was not empty.Comment #8
christinlepson commentedComment #9
mondrakeWow! Great job!
There are some coding standard issues https://www.drupal.org/pift-ci-job/1314875, wrong indentations.
We would a @trigger_error in the deprecated method, and a @legacy test to demonstrate we're still bacward compatible. See core/tests/Drupal/Tests/PhpunitCompatibilityTrait.php and core/tests/Drupal/Tests/PhpunitCompatibilityTraitTest.php for inspiration :)
Comment #10
christinlepson commentedThanks!
Added the @trigger_error and @legacy test and cleaned up indentations for coding standards.
Comment #11
yogeshmpawarSetting back to Needs Review & Triggering bots.
Comment #12
mondrakeNitpick, missing space after comma before
$expectedCode.Better skip these than adding empty, out of scope of this issue. Also in ArgumentsResolverTest.
Other than that, looks great!
Comment #13
christinlepson commentedSorry, I ran code sniffer to fix the indentations and didn't realize it added the empty docs.
This should take care of the empty docs and the missing space.
Comment #14
christinlepson commentedComment #15
mondrakeThanks @clepson!
In general:
1) when you upload a new patch, just set the issue status to 'Needs review'. The testbot will automatically start the test on the newly uploaded patch, without you needing to manually start a test.
2) when uploading a new patch that increments from a previous one, mind adding an interdiff. This helps reviewers looking only at the changes since previous patch, and speeds up reviews.
Other than that, this looks great, RTBC
Comment #16
christinlepson commented@mondrake thanks so much for your direction. I'll keep those in mind going forward, this was my first contribution and you were very helpful.
Comment #17
alexpottUnrelated change
unrelated change
unrelated change
unrelated change
The @deprecated needs a blank line above it as it is not a param.
We also need an @see to the change record.
Needs a a link to the change record.
<3 great to see a deprecation test.
composer run phpcs -- -psorcomposer run phpcbf -- -psI think that would have resulted in less out-of-scope change.Comment #18
christinlepson commentedRemoved unrelated changes, added a blank line above the @deprecated annotation, added an @see to the change record as well as links to the change record in the @trigger_error statement and the @expectedDeprecation annotation of the unit test.
Comment #19
christinlepson commentedComment #20
mondrake#17 has been addressed, and I cannot find anything to nitpick about. Back to RTBC.
Added this issue to the existing CR and modified that to mention deprecation of
getMockandsetExpectedException.Comment #21
alexpottCommitted e82e0c0 and pushed to 8.8.x. Thanks!
Comment #23
mondrakeMore cleanup @ #3060391: Cleanup usage of PHPUnit 4.8 aliased classes.