Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets

@gabesullice Attached is a patch for the work we started in order to test nested includes.

@pixelwhip WOOT! Thank you for getting this going! 👍❤️

Interdiff from #3 to #5?

Do you want reviews already, or do you want to get the patch green first?

1. +++ b/tests/src/Functional/CommentTest.php
   @@ -412,4 +412,11 @@ class CommentTest extends ResourceTestBase {
   +  protected function setUpIncludedAuthorization(array $field_set) {
   +    $this->grantPermissionsToTestedRole(['administer comment types', 'access user profiles']);
   

   Shouldn't this conditionally grant permissions based on the fields in $field_set?

   (In this implementation and all others.)

2. +++ b/tests/src/Functional/NodeTest.php
   @@ -340,4 +340,11 @@ class NodeTest extends ResourceTestBase {
   +    $this->grantPermissionsToTestedRole(['administer users', 'administer permissions']);
   

   I'm confused why this needs administer permissions.

   Also, wouldn't access user profiles be sufficient, isn't administer users overkill?

3. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
   @@ -120,6 +129,53 @@ trait ResourceResponseTestTrait {
   +   *   The ResourceResponse.
   

   Nit: s/The/The expected/

4. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
   @@ -120,6 +129,53 @@ trait ResourceResponseTestTrait {
   +  protected function getExpectedIncludedResourceData(array $field_set, array $request_options) {
   

   Nit: method name says "data", but actually returns a response.

5. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
   @@ -120,6 +129,53 @@ trait ResourceResponseTestTrait {
   +        $collected_responses[] = static::toCollectionResourceResponse(static::toResourceResponses($psr_responses),NULL,TRUE);
   

   Nit: CS violations around the commas.

6. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
   @@ -389,4 +445,41 @@ trait ResourceResponseTestTrait {
   +   * @param null $relationship_field_name
   

   Nit: string|null

7. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -940,6 +962,10 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      $actual_document = Json::decode((string) $actual_response->getBody());
   +      $expected_document = $expected_resource_response->getResponseData();
   +      $this->assertSame($expected_resource_response->getStatusCode(), $actual_response->getStatusCode());
   +      $this->assertSameDocument($expected_document, $actual_document);
          // @todo uncomment this assertion in https://www.drupal.org/project/jsonapi/issues/2929428
          // Dynamic Page Cache miss because cache should vary based on the
          // 'include' query param.
   @@ -955,10 +981,6 @@ abstract class ResourceTestBase extends BrowserTestBase {
   
   @@ -955,10 +981,6 @@ abstract class ResourceTestBase extends BrowserTestBase {
          //  $expected_cacheability->getCacheMaxAge() === 0 ? 'UNCACHEABLE' : 'MISS'
          //);
          // @codingStandardsIgnoreEnd
   -      $this->assertSame($expected_resource_response->getStatusCode(), $actual_response->getStatusCode());
   -      $expected_document = $expected_resource_response->getResponseData();
   -      $actual_document = Json::decode((string) $actual_response->getBody());
   -      $this->assertSameDocument($expected_document, $actual_document);
   

   Why move this?

8. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1175,6 +1177,56 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +  protected function getExpectedIncludeResponse(array $field_set, array $request_options) {
   +    $individual_response = $this->getExpectedGetIndividualResourceResponse();
   
   @@ -2016,36 +2064,23 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -      // The expected response is based on the expected individual response for
   -      // this resource type, it will then be decorated using the related
   -      // response data.
   -      $expected_document = $this->getExpectedDocument();
   -      // Update the expected 'self' link with expected include query parameter.
   -      $expected_document['links']['self'] = $url->setAbsolute()->toString();
   

   This just moved some of the existing logic into a new helper method 👍

9. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1175,6 +1177,56 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    // updated self link as is.
   

   Nit: s/self/'self'/ (to make it slightly less ambiguous/slightly easier to understand)

10. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -2016,36 +2064,23 @@ abstract class ResourceTestBase extends BrowserTestBase {
    -      $expected_cacheability = (new CacheableMetadata())
    -        ->setCacheContexts($this->getExpectedCacheContexts())
    -        ->setCacheTags($this->getExpectedCacheTags());
    -      $expected_response = static::decorateExpectedResponseForIncludedFields(
    -        (new ResourceResponse($expected_document))->addCacheableDependency($expected_cacheability),
    
    @@ -2054,62 +2089,31 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +  protected function getExpectedGetIndividualResourceResponse($status_code = 200) {
    +    $resource_response = new ResourceResponse($this->getExpectedDocument(), $status_code);
    +    $cacheability = new CacheableMetadata();
    +    $cacheability->setCacheContexts($this->getExpectedCacheContexts());
    +    $cacheability->setCacheTags($this->getExpectedCacheTags());
    +    return $resource_response->addCacheableDependency($cacheability);
       }
    

    Why didn't I think of this helper method? Surprisngly elegant! 😍

11. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -2149,28 +2153,83 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +   * Check access for the given operation, field and entity.
    

    s/given operation, field and entity./given field operation on the given entity./

12. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -2149,28 +2153,83 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +   * Gets a list of nested include paths.
    

    I think this is meant to mean *all* nested include paths?

13. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -2183,7 +2242,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
    -  protected function isReferenceFieldDefinition(FieldDefinitionInterface $field_definition) {
    +  protected static function isReferenceFieldDefinition(FieldDefinitionInterface $field_definition) {
    

    ❤️

Note that #2953207: Can't get the right target type when filtering on relationship with bundle-specific target entity type is making changes to fix a bug, but in doing so it introduced another bug. Fortunately they were caught by \Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest::testRead(). The test coverage that this issue is adding will help ensure with more reliability that such regressions won't occur :)

#14.1: oohh, I think I like that!

In #10, I mentioned de-scoping nested sparse field sets. @Wim Leers, what do you think?

I can't see a practical way to do that in an automated way.

I'm open to that.

But here's an idea: what if we only do this for entity types implementing EntityOwnerInterface? Then the tested secondary resource type for which we're testing sparse fieldsets would always be User, which would make it easy to test only those well-known entity types' fields.

Because you're absolutely right of course: not every entity type has an entity reference (relationship), and if they do, it's often to config entity types (e.g. Node -> NodeType, Term -> Vocabulary). By only doing it for entity types with required relationships to User entities, you're still testing >1 use case, yet keeping it simpler to test.

What do you think?

I was initially confused by the need for #17+#19+#20 to make this patch pass tests again. But then I understood: the patches before then were applied to HEAD, and since then #2953318: Comprehensive JSON API integration test coverage phase 4: collections, filtering and sorting landed! Hence the need for quite a bit of rebasing.

#21: I'm glad you found #2976108: Spec Compliance: Impossible to include related resources when relationship field is not in a request's sparse fieldset and created an issue for it! 👌

I'm of course super glad that we now have test coverage for sparse field sets!

Re-reviewed the entire patch.

1. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -2340,16 +2341,36 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // @todo uncomment the following line in https://www.drupal.org/project/jsonapi/issues/2976108
   +      //$field_sets['nested_empty_fieldset'] = $field_sets['empty'];
   

   👌

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -2340,16 +2341,36 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      $field_sets['nested_fieldset_with_owner_fieldset'] = ['uid'];
   

   This specified that the sparse fieldset should be [uid].

   +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -2340,16 +2341,36 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +        $query['fields[user--user]'] = 'name';
   +        $query['include'] = 'uid';
   

   Yet this hardcodes it to name. Easy fix!

   Oh wait … this too is blocked on #2976108: Spec Compliance: Impossible to include related resources when relationship field is not in a request's sparse fieldset! So, for now, the solution is to not hardcode but respect the specified fieldset, yet make that fieldset temporarily ['uid'], until #2976108 lands, then we can make it ['name'] again! (Or better yet, something like ['name','preferred_langcode']. Because then we're testing multiple fields being requested in a sparse fieldset.)

   I made the necessary changes and added a @todo.

3. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
   @@ -392,4 +450,50 @@ trait ResourceResponseTestTrait {
   +   * Get a generic forbidden response.
   
   +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1501,6 +1508,56 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +   * Get an expected ResourceResponse with includes for the given field set.
   

   Nit: Gets.

Still green, let's get this done so we can move on to #2972808: Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships, the final test coverage piece! This patch is pure test coverage, so there's zero risk of BC breaks by committing this!

(I'll fix the CS violations on commit. See attached interdiff.)