Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Havin rolled this patch, I couldn't help but notice that:

  1. \Drupal\rest\Tests\AuthTest is also completely incorporated by #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, so deleted that too
  2. \Drupal\rest\Tests\CsrfTest is also completely incorporated by #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, so deleted that too
  3. \Drupal\rest\Tests\NodeTest is mostly incorporated by #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, so deleted the parts that have been incorporated
  4. \Drupal\rest\Tests\PageCacheTest is mostly incorporated by #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, if we just add some cache tag test coverage to #2737719, we can delete that too

Working on points 3 and 4 over in that other issue.

Wim Leers’s picture

FileSize
6.07 KB
68.07 KB
Wim Leers’s picture

catch’s picture

What about hal module's tests? Can any of those be consolidated?

Wim Leers’s picture

+1! Will do that in this patch too.

Wim Leers’s picture

FileSize
84.32 KB
11.76 KB
  1. Can't remove \Drupal\hal\Tests\FileDenormalizeTest + \Drupal\Tests\hal\Kernel\FileNormalizeTest because adding File entity test coverage is not done by #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, that's for #2824572: Write EntityResourceTestBase subclasses for every other entity type. to add.
  2. \Drupal\Tests\hal\Kernel\DenormalizeTest is testing edge cases for "type" link handling. All other test methods (the majority) can be removed. Although I think it's better to keep its testMarkFieldForDeletion() method too, that's explicit test coverage for an edge case, where #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method only added implicit test coverage for that. We should eventually move that into \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase too, but that'd make #2737719 even biggger, so I think it's fine to defer that to a follow-up.
  3. \Drupal\Tests\hal\Kernel\EntityNormalizeTest tests a subset of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, removed completely
  4. \Drupal\Tests\hal\Kernel\EntityTranslationNormalizeTest + \Drupal\Tests\hal\Kernel\NormalizeTest are testing translation support, which actually is not properly supported at all yet, for that we have #2135829: [PP-1] EntityResource: translations support
Wim Leers’s picture

Title: [PP-1] Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase » Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase
Status: Postponed » Needs review
FileSize
84.32 KB
Wim Leers’s picture

Will roll a separate 8.3.x patch first thing tomorrow.

Wim Leers’s picture

There were two changes in 8.3.x that prevented the patch from applying to 8.3.x too:

  1. #1841474: Convert REST tests to Guzzle (would also help with core moving from WTB to BTB) already being committed to 8.3.x but not 8.2.x
  2. the same thing I encountered at #2737719-134: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.3:
  3. The status field of Comment entities is validated before other fields in 8.3.x, due to [#2810381.

Reuploading the same 8.2.x patch as in #7, and a separate patch for 8.3.x.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Looking at both branch patches and they both look good! RTBC!

@Wim Leers thanks for cleaning this out.

  • catch committed 8f53fb1 on 8.3.x
    Issue #2824576 by Wim Leers: Delete old REST test coverage: (Read|Create...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x and 8.2.x, thanks!

  • catch committed ec3d56c on 8.2.x
    Issue #2824576 by Wim Leers: Delete old REST test coverage: (Read|Create...
Wim Leers’s picture

Status: Fixed » Closed (fixed)

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