Closed (fixed)
Project:
Drupal core
Version:
8.2.x-dev
Component:
rest.module
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
3 Nov 2016 at 10:42 UTC
Updated:
15 Dec 2016 at 16:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
wim leersHavin rolled this patch, I couldn't help but notice that:
\Drupal\rest\Tests\AuthTestis also completely incorporated by #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, so deleted that too\Drupal\rest\Tests\CsrfTestis also completely incorporated by #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, so deleted that too\Drupal\rest\Tests\NodeTestis 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\Drupal\rest\Tests\PageCacheTestis 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 tooWorking on points 3 and 4 over in that other issue.
Comment #3
wim leersAddressed point 4 at #2737719-113: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, so rerolling this patch to also remove
PageCacheTest.Comment #4
wim leersAddressed point 3 at #2737719-114: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, so rerolling this patch to also remove
NodeTest. In doing so, I discovered #2824827: \Drupal\hal\Normalizer\ContentEntityNormalizer::denormalize() fails with fatal PHP error when bundles are missing from link relation types, which again shows why we need to test all edge cases against all formats.Comment #5
catchWhat about hal module's tests? Can any of those be consolidated?
Comment #6
wim leers+1! Will do that in this patch too.
Comment #7
wim leers\Drupal\hal\Tests\FileDenormalizeTest+\Drupal\Tests\hal\Kernel\FileNormalizeTestbecause addingFileentity 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.\Drupal\Tests\hal\Kernel\DenormalizeTestis testing edge cases for "type" link handling. All other test methods (the majority) can be removed. Although I think it's better to keep itstestMarkFieldForDeletion()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\EntityResourceTestBasetoo, but that'd make #2737719 even biggger, so I think it's fine to defer that to a follow-up.\Drupal\Tests\hal\Kernel\EntityNormalizeTesttests a subset of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, removed completely\Drupal\Tests\hal\Kernel\EntityTranslationNormalizeTest+\Drupal\Tests\hal\Kernel\NormalizeTestare testing translation support, which actually is not properly supported at all yet, for that we have #2135829: EntityResource: translations supportComment #8
wim leers#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method landed!
Let's see if this patch still applies.
Comment #9
wim leersWill roll a separate 8.3.x patch first thing tomorrow.
Comment #10
wim leersThere were two changes in 8.3.x that prevented the patch from applying to 8.3.x too:
Reuploading the same 8.2.x patch as in #7, and a separate patch for 8.3.x.
Comment #11
tedbowLooking at both branch patches and they both look good! RTBC!
@Wim Leers thanks for cleaning this out.
Comment #13
catchCommitted/pushed to 8.3.x and 8.2.x, thanks!
Comment #15
wim leersYay!