Problem/Motivation

In #3053417: Clean up PhpunitCompatibilityTrait it was discussed to deprecate \Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException().

Proposed resolution

Do that, and replace calls with $this->expectException and $this->expectExceptionMessage.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Issue tags: +Novice

In #3053417: Clean up PhpunitCompatibilityTrait it was discussed to deprecate \Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException().

Do that, and replace calls with $this->expectException and $this->expectExceptionMessage.

mondrake’s picture

Issue summary: View changes
sershevchyk’s picture

Assigned: Unassigned » sershevchyk
christinlepson’s picture

StatusFileSize
new337.42 KB

Deprecated the \Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException() function and replaced a total of 517 calls.

197 single parameter calls were replaced with $this->expectException and 320 calls replaced with $this->expectException and $this->expectExceptionMessage.

christinlepson’s picture

christinlepson’s picture

StatusFileSize
new337.51 KB

Drupal\Tests\jsonapi\Kernel\Context\FieldResolverTest needed an if statement to call $this->expectExceptionMessage only when the message was not empty.

christinlepson’s picture

Status: Active » Needs review
mondrake’s picture

Status: Needs review » Needs work

Wow! 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 :)

christinlepson’s picture

StatusFileSize
new342.18 KB

Thanks!
Added the @trigger_error and @legacy test and cleaned up indentations for coding standards.

yogeshmpawar’s picture

Assigned: sershevchyk » Unassigned
Status: Needs work » Needs review

Setting back to Needs Review & Triggering bots.

mondrake’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTraitTest.php
@@ -21,6 +21,20 @@ public function testGetMock() {
+    throw new \Exception($expectedMessage,$expectedCode);

Nitpick, missing space after comma before $expectedCode.


+++ b/core/tests/Drupal/Tests/Core/Cache/Context/CacheContextsManagerTest.php
@@ -137,12 +137,18 @@ public function providerTestInvalidCalculatedContext() {
+  /**
+   *
+   */

Better skip these than adding empty, out of scope of this issue. Also in ArgumentsResolverTest.

Other than that, looks great!

christinlepson’s picture

StatusFileSize
new340.88 KB

Sorry, 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.

christinlepson’s picture

Status: Needs work » Needs review
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

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

christinlepson’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
  1. +++ b/core/modules/migrate_drupal/migrate_drupal.install
    @@ -2,7 +2,7 @@
     /**
      * @file
    - * Contains install and update functions for Migrate Drupal
    + * Contains install and update functions for Migrate Drupal.
      */
    

    Unrelated change

  2. +++ b/core/modules/migrate_drupal/tests/src/Unit/FieldDiscoveryTest.php
    @@ -178,7 +178,7 @@ public function getBundleFieldsData() {
    -            'user_field_1' => ['field_info_key' => 'user_field_1_data'],
    +          'user_field_1' => ['field_info_key' => 'user_field_1_data'],
    

    unrelated change

  3. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
    @@ -162,9 +163,11 @@ public function testTimestamp($input, array $initial, array $transform) {
        * @param array $initial
    -   *   @see testTimestamp()
    +   *
    +   * @see testTimestamp()
        * @param array $transform
    -   *   @see testTimestamp()
    +   *
    +   * @see testTimestamp()
        *
        * @dataProvider providerTestDateTimestamp
        */
    @@ -180,12 +183,15 @@ public function testDateTimestamp($input, array $initial, array $transform) {
    
    @@ -180,12 +183,15 @@ public function testDateTimestamp($input, array $initial, array $transform) {
        *
        * @param \Drupal\Component\Datetime\DateTimePlus $date
        *   DateTimePlus to test.
    +   *
        * @input mixed $input
        *   The original input passed to the test method.
        * @param array $initial
    -   *   @see testTimestamp()
    +   *
    +   * @see testTimestamp()
        * @param array $transform
    -   *   @see testTimestamp()
    +   *
    +   * @see testTimestamp()
        */
    

    unrelated change

  4. +++ b/core/tests/Drupal/Tests/Component/FileCache/FileCacheFactoryTest.php
    @@ -157,7 +158,7 @@ public function configurationDataProvider() {
    -  ];
    +    ];
    

    unrelated change

  5. +++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTrait.php
    @@ -97,8 +97,11 @@ public function getMock($originalClassName, $methods = [], array $arguments = []
        *   The expected exception code.
    +   * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.
    +   *   Backward compatibility for PHPUnit 4 will no longer be supported.
    

    The @deprecated needs a blank line above it as it is not a param.

    We also need an @see to the change record.

  6. +++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTrait.php
    @@ -97,8 +97,11 @@ public function getMock($originalClassName, $methods = [], array $arguments = []
    +    @trigger_error('\Drupal\Tests\PhpunitCompatibilityTrait:setExpectedException() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Backward compatibility for PHPUnit 4 will no longer be supported.', E_USER_DEPRECATED);
    

    Needs a a link to the change record.

  7. +++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTraitTest.php
    @@ -21,6 +21,20 @@ public function testGetMock() {
    +  public function testSetExpectedException() {
    

    <3 great to see a deprecation test.

  8. Note when running coding standards if you want to only run the ones that core currently implements use composer run phpcs -- -ps or composer run phpcbf -- -ps I think that would have resulted in less out-of-scope change.
christinlepson’s picture

StatusFileSize
new339.12 KB
new5.2 KB

Removed 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.

christinlepson’s picture

Status: Needs work » Needs review
mondrake’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

#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 getMock and setExpectedException.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e82e0c0 and pushed to 8.8.x. Thanks!

  • alexpott committed e82e0c0 on 8.8.x
    Issue #3059090 by clepson, mondrake, alexpott: Deprecate \Drupal\Tests\...
mondrake’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.