Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets

#2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets landed!

I think that a good starting point could be to move much of \Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest into ResourceTestBase, to e.g. do testGetCollection().

Note that I do not intend to take this on for the time being. I think this is more in @e0ipso and @gabesullice's ball park — they're more deeply familiar with how these are supposed to work, which edge cases are worth testing, and so on.

Interpret comprehensive as as comprehensive as is reasonable and useful. It's impossible to test 100% of possible cases. For example, ResourceTestBase::testPatchIndividual() does this:

    // Multi-value field: remove item 0. Then item 1 becomes item 0.
    $doc_multi_value_tests = $this->getPatchDocument();
    $doc_multi_value_tests['data']['attributes']['field_rest_test_multivalue'] = $this->entity->get('field_rest_test_multivalue')->getValue();
    $doc_remove_item = $doc_multi_value_tests;
    unset($doc_remove_item['data']['attributes']['field_rest_test_multivalue'][0]);
    $request_options[RequestOptions::BODY] = Json::encode($doc_remove_item, 'api_json');
    $response = $this->request('PATCH', $url, $request_options);
    $this->assertResourceResponse(200, FALSE, $response, Cache::mergeTags($this->entity->getCacheTags(), ['http_response']), $this->getExpectedCacheContexts());
    $this->assertSame([0 => ['value' => 'Two']], $this->entityStorage->loadUnchanged($this->entity->id())->get('field_rest_test_multivalue')->getValue());

    // Multi-value field: add one item before the existing one, and one after.
    $doc_add_items = $doc_multi_value_tests;
    $doc_add_items['data']['attributes']['field_rest_test_multivalue'][2] = ['value' => 'Three'];
    $request_options[RequestOptions::BODY] = Json::encode($doc_add_items);
    $response = $this->request('PATCH', $url, $request_options);
    $this->assertResourceResponse(200, FALSE, $response, Cache::mergeTags($this->entity->getCacheTags(), ['http_response']), $this->getExpectedCacheContexts());
    $expected = [
      0 => ['value' => 'One'],
      1 => ['value' => 'Two'],
      2 => ['value' => 'Three'],
    ];
    $this->assertSame($expected, $this->entityStorage->loadUnchanged($this->entity->id())->get('field_rest_test_multivalue')->getValue());

That's testing an artificial field by first removing a particular item, and then adding one item before and item after. It's not doing this for every possible field. It's not doing this for an actual field. It's testing the mechanics.

I think that for the JSON API-specific capabilities, testing the mechanics is the most important thing.

I'd choose some good representatives of each case and call it tested.

Exactly :) This sounds perfect!

Go Gabe! 🤘

1. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1422,4 +1425,31 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +  protected function doTestSparseFieldSets($request_options) {
   ...
   +      $url = Url::fromRoute(
   +        sprintf('jsonapi.%s.individual', static::$resourceTypeName),
   +        [static::$entityTypeId => $this->entity->uuid()],
   +        ['query' => ['fields[' . static::$resourceTypeName . ']' => implode(',', $field_set)]]
   +      );
   

   Rather than regenerating a URL from scratch, you can pass the $url that the caller was already working with.

   Then you can just do $url->setOption('query', …);.

   Less code!

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1422,4 +1425,31 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      $this->setUpAuthorization('GET');
   

   This is not necessary: it already happened int he caller's code.

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1422,4 +1425,31 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // Cache MISS because cache should vary based on the 'field' query param.
   +      $this->assertResourceResponse(200, FALSE, $response, $this->getExpectedCacheTags(), $this->getExpectedCacheContexts(), FALSE, 'MISS');
   

   😍

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -463,6 +466,19 @@ abstract class ResourceTestBase extends BrowserTestBase {
+  protected function assertDocumentResponse(array $document, ResponseInterface $response) {

Rather than adding this, I think we should update assertResourceResponse(), shouldn't we?

You're also of course right that assertResourceResponse() is a misnomer, it should be renamed to assertDocumentResponse(). Same for the error one.

(FYI: I think #15 can also happen in a follow-up — probably that's even better, because then this patch is only adding test coverage rather than refactoring existing test coverage. Just add a @todo to the new assertDocumentResponse()!)

Nice progress! And fewer failures :)

This looks GREAT!

1. +++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
   @@ -168,10 +168,13 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
   -      $unique_key = $rasterized_include['data'] === FALSE ?
   -        $rasterized_include['meta']['errors'][0]['detail'] :
   -        $rasterized_include['data']['type'] . ':' . $rasterized_include['data']['id'];
   -      $unique_includes[$unique_key] = $include;
   +      if ($rasterized_include['data'] === FALSE) {
   +        $unique_includes[] = $include;
   +      }
   +      else {
   +        $unique_key = $rasterized_include['data']['type'] . ':' . $rasterized_include['data']['id'];
   +        $unique_includes[$unique_key] = $include;
   +      }
   

   Is this a pure simplification or a bugfix?

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -461,7 +461,10 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -        $this->assertSame($expected_body, (string) $response->getBody());
   +        $this->assertSame(
   +          static::sortJsonString($expected_body),
   +          static::sortJsonString((string) $response->getBody())
   +        );
   

   The intent here was that if you need to do sorting/deep analysis of the response body, that you pass FALSE to skip testing of the response body here (i.e. skip this assertion).

   Then you can just write your own assertions after doing $this->assertResourceResponse():

   $doc = Json::decode((string) $response->getBody());
   // assert things in $doc
   

   Would you rather have it not work that way? I can live with that.

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -474,7 +477,8 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -      $this->assertSame($expected_cache_contexts, explode(' ', $response->getHeader('X-Drupal-Cache-Contexts')[0]));
   +      $optimized_expected_cache_contexts = \Drupal::service('cache_contexts_manager')->optimizeTokens($expected_cache_contexts);
   +      $this->assertSame($optimized_expected_cache_contexts, explode(' ', $response->getHeader('X-Drupal-Cache-Contexts')[0]));
   

   This means that the expected cache contexts may be a superset of what you get back. I'd much prefer to update getExpectedCacheContexts(), so that we can again do the simple straight comparison, so that what a specific test is expecting, is also actually what the response contains!

4. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -715,11 +734,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
        // Sort the serialization data first so we can do an identical comparison
        // for the keys with the array order the same (it needs to match with
        // identical comparison).
   -    $expected = $this->getExpectedDocument();
   -    static::recursiveKsort($expected);
   -    $actual = Json::decode((string) $response->getBody());
   -    static::recursiveKsort($actual);
   -    $this->assertSame($expected, $actual);
   

   But then we should just remove this altogether, and update

       // 200 for well-formed GET request. Page Cache hit because of HEAD request.
       // Same for Dynamic Page Cache hit.
       $response = $this->request('GET', $url, $request_options);
       $this->assertResourceResponse(200, FALSE, $response, …
   

   to:

       // 200 for well-formed GET request. Page Cache hit because of HEAD request.
       // Same for Dynamic Page Cache hit.
       $response = $this->request('GET', $url, $request_options);
       $this->assertResourceResponse(200, Json::encode($this->getExpectedDocument()), $response, …
   

5. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -797,6 +812,19 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    catch (\Exception $e) {
   ...
   +      $this->markTestSkipped("Remove this try/catch in https://www.drupal.org/project/jsonapi/issues/2930217 and/or https://www.drupal.org/project/jsonapi/issues/2940339");
   

   Can we catch a more specific exception and then not mark this test as skipped, but rather complete the remainder of the test coverage?

6. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1573,4 +1615,286 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +  protected function doTestSparseFieldSets(Url $url, array $request_options) {
   ...
   +  public function testIncluded() {
   

   Why does one have its own test method, and the other is just tested as part of testGetIndividual()?

7. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1573,4 +1615,286 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      'some' => array_slice($field_names, floor(count($field_names) / 2)),
   

   This means that if content entity types get more base fields, the test coverage would also be testing different things. Probably okay, but not ideal. Of course, testing every possible combination makes even less sense.

   However, wouldn't a more particularly selected subset make more sense? E.g. not listing the uuid field here should still result in the id entry per the JSON API standard, but then would probably have to omit the attributes.uuid entry?

8. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1573,4 +1615,286 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // 'self' link should include the 'fields' query param.
   +      $expected_body['links']['self'] = $url->setAbsolute()->toString();
   

   ❤️

9. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1573,4 +1615,286 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // Cache MISS because cache should vary based on the 'field' query param.
   ...
   +    // Test cache HIT for a query with the same field set.
   

   ❤️

10. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1573,4 +1615,286 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +    $field_names = isset($individual_response_document['data']['relationships'])
    

    Nit: $relationship_field_names.

11. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1573,4 +1615,286 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +  protected static function decorateExpectedResponseForIncludedFields(array &$expected_response, array $related_responses) {
    ...
    +        // If any of the related response documents had top-level errors, we
    +        // should later expect the document to have 'meta' errors too.
    ...
    +        if (array_key_exists('type', $related_data) && array_key_exists('id', $related_data)) {
    

    ❤️

12. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1573,4 +1615,286 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +  protected function getRelatedResponses(array $links, array $request_options) {
    

    ❤️

13. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1573,4 +1615,286 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +      $cacheable_metadata = new CacheableMetadata();
    

    Nit: s/cacheable_metadata/cacheability/

#29.7: I'm saying that if you specify the sparse fieldset title,field_tags, that you should get:

{
  id: …,
  attributes: {
    title: …,
    field_tags: …
  },
  …
}

Note how attributes.uuid is absent (because not listed in the sparse fieldset), even though id is/must be always included. So my point was that that is probably an edge case worth testing.

1. +++ b/tests/src/Functional/NodeTest.php
   @@ -313,7 +313,7 @@ class NodeTest extends ResourceTestBase {
   -    $expected = [
   +    $expected_document = [
   
   @@ -330,7 +330,7 @@ class NodeTest extends ResourceTestBase {
   -    $this->assertResourceResponse(403, Json::encode($expected), $response);
   +    $this->assertResourceResponse(403, $expected_document, $response);
   

   This is a bit too much of a refactor to include in this patch IMO: it's touching pretty much all existing integration test coverage! Let's split this off into a separate issue, and we can get it committed very quickly there.

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -428,8 +428,9 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +   *   The expected document or NULL if there should be not be a response body.
   

   Nit: s/be not be/not be/ :)

   Other than that: ❤️

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -460,11 +461,14 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +          $this->assertTrue(is_null($response_document));
   

   Nit: assertNull()

4. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -503,16 +506,15 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +  protected function assertSameDocument(array $expected_document, array $actual_document) {
   

   ❤️

5. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1793,6 +1762,22 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +   * Returns an array of sparse fields sets to test.
   ...
   +  protected function getSparseFieldSets() {
   

   This means every subclass can test different sets of sparse field sets. Handy for regression tests. Cool!

@gabesullice created #2949136: Refactor assertResourceResponse test method to be more flexible. for #34.1.

#2949136: Refactor assertResourceResponse test method to be more flexible. is in!

That should make this patch quite a bit smaller and simpler!

After that, I think we should commit this if @Wim Leers and @e0ipso agree. That will unblock #2940342: Cacheability metadata on an entity fields' properties is lost.

Then I can add coverage for related routes and collections.

👍

Final review time!

1. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // Cache MISS because cache should vary based on the 'field' query param.
   ...
   +      // Cache MISS because cache should vary based on the 'include' query
   

   Nit: s/Cache MISS/Dynamic Page Cache miss/

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    // Test cache HIT for a query with the same field set.
   

   Nit: s/cache HIT/Dynamic Page Cache hit/

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    $relationship_field_names = isset($individual_response_document['data']['relationships'])
   

   Note that this will only return the accessible relationship fields!

   For example, UserTest does not list roles in \Drupal\Tests\jsonapi\Functional\UserTest::getExpectedDocument() because the GET test coverage can be performed with just the access user profiles permission. To view the roles field, one must have administer users permission.

   This is fine, but it does mean we're not testing everything. Wanted you to be aware of that.

   Theoretically, that also means this should be renamed to $accessible_relationship_field_names.

4. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    // If there are no relationships, we can't include anything.
   

   no accessible relationships

5. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      $about_half_the_fields = floor(count($relationship_field_names) / 2);
   

   Love this variable name :P

6. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // The expected response is based of the expected individual response.
   

   s/of/on/
   s/individual response/individual responses/

   I think?

7. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      $expected_response = [
   +        'document' => $this->getExpectedDocument(),
   +        'cacheability' => (new CacheableMetadata())
   +          ->setCacheContexts($this->getExpectedCacheContexts())
   +          ->setCacheTags($this->getExpectedCacheTags()),
   +      ];
   ...
   +   *   The expected response. This should be an array with a 'document' key and
   +   *   corresponding JSON API document array and a 'cacheability' key with a
   

   Rather than this work-around for the fact that PHP is a limited language where you don't have the equivalent of pair, you could also use the \Drupal\jsonapi\ResourceResponse value object which allows you to pass exactly this kind of data :)

8. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // param. It's not possible to know the order of the included resources,
   +      // thus it is not possible to assert the expected body.
   ...
   +        $this->assertSame(
   +          Json::encode(static::sortResourceCollection($expected_response['document']['included'])),
   +          Json::encode(static::sortResourceCollection($response_document['included']))
   +        );
   ...
   +  protected static function sortResourceCollection(array &$resources) {
   

   Can this now not use \Drupal\Tests\jsonapi\Functional\ResourceTestBase::assertSameDocument()?

9. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +   * This adds the expected included to the expected document and also builds
   

   s/included/includes/

10. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +   * the expected cache metadata. It does so based of responses from the related
    

    s/cache metadata/cacheability/

11. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +   *   corresponding cacheable metadata object.
    

    s/cacheable metadata/CacheableMetadata/

12. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +      /* @var \Drupal\Core\Cache\CacheableMetadata $related_cacheability */
    +      $related_cacheability = $related_response['cacheability'];
    

    The annotation does not match.

13. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +  protected static function extractRelatedLinks(array $field_names, array $document) {
    

    static 👍

14. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +      $related_response = $this->request('GET', Url::fromUri($links[$field_name]), $request_options);
    

    Nice!

15. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1586,4 +1592,292 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +      $cacheability = new CacheableMetadata();
    ...
    +      $related_responses[$field_name] = [
    +        'document' => Json::decode($related_response->getBody()),
    +        'cacheability' => $cacheability,
    +      ];
    

    Same as above: you could use \Drupal\jsonapi\ResourceResponse to make it easier to pass this around!

This looks lovely now!

1. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,305 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +  /**
   +   * Tests sparse field sets.
   +   *
   +   * @param \Drupal\Core\Url $url
   +   *   The base URL with which to test includes.
   +   * @param array $request_options
   +   *   Request options to apply.
   +   */
   ...
   +  /**
   +   * Tests included resources.
   +   *
   +   * @param \Drupal\Core\Url $url
   +   *   The base URL with which to test includes.
   +   * @param array $request_options
   +   *   Request options to apply.
   +   */
   ...
   +   * @param array $request_options
   +   *   Request options to apply.
   

   ❓ In ::request() we have a

   @see \GuzzleHttp\ClientInterface::request()
   

   Perhaps we want the same here? Not sure.

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1586,4 +1592,305 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      $this->assertResourceResponse(
   +        200,
   +        $expected_document,
   +        $response,
   +        $this->getExpectedCacheTags(),
   +        $this->getExpectedCacheContexts(),
   +        FALSE,
   +        'MISS'
   +      );
   

   🤐 Supernit: this is technically complying with the coding standards, but is hugely distracting. We put this on a single line everywhere else. I think we should do the same here.

Tests should pass when #2930217: getRelated() not working when related entity field is empty lands.

After that, I think we should commit this if @Wim Leers and @e0ipso agree. That will unblock #2940342: Cacheability metadata on an entity fields' properties is lost.

Then I can add coverage for related routes and collections.

That means we need a phase 4 issue and an issue title+summary update. The phase 4 issue should also be added to #2931785: The path for JSON API to core.

WFM!

Pinged @e0ipso because #2930217: getRelated() not working when related entity field is empty is blocking a commit of this issue, and this issue is blocking #2940342: Cacheability metadata on an entity fields' properties is lost.

Nice, thanks! :)

Moving back to NW per #44 — @gabesullice is planning more work here :)

Looking great!

1. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -659,7 +659,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    $request_options = NestedArray::mergeDeep($request_options, $this->getAuthenticationRequestOptions('GET'));
   +    $request_options = NestedArray::mergeDeep($request_options, $this->getAuthenticationRequestOptions());
   

   Seems like an accidental change?

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -832,6 +832,193 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    // Builds an array of expected responses, keyed by relationship field name.
   +    $expected_related_responses = $this->getExpectedRelatedResponses($url, $individual_response, $request_options, $accessible_relationship_field_names);
   +    // Fetches actual responses as an array keyed by relationship field name.
   +    $related_responses = $this->getRelatedResponses($individual_response, $request_options, $accessible_relationship_field_names);
   +    foreach ($accessible_relationship_field_names as $accessible_relationship_field_name) {
   ...
   +  protected function getExpectedRelatedResponses(Url $individual_url, ResourceResponse $individual_response, array $request_options, array $relationship_field_names) {
   

   ❤️

   This is of course the central part of this patch, and it takes some reading, but then it totally makes sense! 👏

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -832,6 +832,193 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +        // Relationship data is accessible if the field is accessible, but the
   +        // individual targeted resource still may be inaccessible.
   

   Glad to see this being called out explicitly!

   But I think you meant to add something like "So we must request the individual resource for each resource related identifier, so that we can construct the expected 'related' response."

4. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -832,6 +832,193 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // We need to extract the document so that we can make some modifcations.
   

   s/modifcations/modifications/

   Also: which modifications?

5. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -832,6 +832,193 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +  protected static function mergeResourceResponses(array $responses, $self_link) {
   

   It seems that this method along with the toResourceResponse() etc helper method really would benefit from being moved out of this base test class.

   Because this class is growing to an enormous size.

   What do you think about moving them all to a ResourceResponseTestTrait?

6. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -832,6 +832,193 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    // 'self' link since it implies support for the route.
   

   "since it implies support for the route" -> not sure how to interpret this. Could you try to clarify that comment?

7. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1788,63 +1967,231 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +   * @todo Remove URL and request options parameters and derived fields from the
   +   *   entity instead of a request.
   

   Nit: Missing a verb. "and get derived", I think?

8. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1788,63 +1967,231 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +   *   The type of link to build. Either 'relationship' or 'related'.
   

   s/build/get/

9. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1788,63 +1967,231 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -  protected static function extractRelatedLinks(array $field_names, array $document) {
   ...
   +  protected static function extractLinks(array $link_paths, array $document) {
   

   ❤️

10. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1788,63 +1967,231 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +  protected function getRelatedResourceResponses(ResourceResponse $individual_response, array $request_options, array $relationship_field_names) {
    +    return array_map([static::class, 'toResourceResponse'], $this->getRelatedResponses($individual_response, $request_options, $relationship_field_names));
    +  }
    ...
    +  protected function getResourceResponses(array $links, array $request_options) {
    +    return array_map([$this, 'toResourceResponse'], $this->getResponses($links, $request_options));
    +  }
    

    It seems these could use one and the same helper: a

    static function toResourceResponses(ResponseInterface[] $responses) {
      return array_map([static::class,
     'toResourceResponse'],
     $responses);
    }

    ?

11. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1788,63 +1967,231 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +   * @return array
    ...
    +    return $this->getResponses($links, $request_options);
    ...
    +   * @return array
    ...
    +    return $this->getResourceResponses($links, $request_options);
    

    Could document a more specific type.

    Also, why does one return ResponseInterface[] and the other ResourceResponse[]?

12. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1788,63 +1967,231 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +  protected function getRelatedResponses(ResourceResponse $individual_response, array $request_options, array $relationship_field_names) {
    ...
    +  protected function getRelationshipResponses(ResourceResponse $individual_response, array $request_options, array $relationship_field_names) {
    

    ❤️

13. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1788,63 +1967,231 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +   * Maps a response object to a JSON API ResourceResponse.
    

    s/response/PSR response/

14. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1788,63 +1967,231 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +   * cacheability data and to have easier access to the JSON API document as an
    

    s/cacheability data/cacheable responses/

15. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1788,63 +1967,231 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +  protected static function toResourceResponse(ResponseInterface $response) {
    

    ❤️

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -832,6 +832,193 @@ abstract class ResourceTestBase extends BrowserTestBase {
+  public function testRelated() {

For consistency with testGetIndividual(), this should be renamed to testGetRelated().

In the future, we'll also want to add testPostRelated(), testPatchRelated() and testDeleteRelated().

#56: 🤦‍♂️ — of course, I misread \Drupal\jsonapi\Routing\Routes::routes(), sorry!
#57++
#58++

1. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
   @@ -0,0 +1,335 @@
   + * Trait ResourceResponseTestTrait.
   ...
   + * @package Drupal\Tests\jsonapi\Functional
   

   Nit: these lines can be removed.

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -840,6 +846,255 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +  public function testRelated() {
   +    $request_options = [];
   +    $request_options[RequestOptions::HEADERS]['Accept'] = 'application/vnd.api+json';
   +    $request_options = NestedArray::mergeDeep($request_options, $this->getAuthenticationRequestOptions());
   +    $this->doTestRelated($request_options);
   +    $this->setUpAuthorization('GET');
   +    $this->doTestRelated($request_options);
   +  }
   

   😍 — so elegant!

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -840,6 +846,255 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +   * Unlike the "related" routes, relationship routes only return information
   +   * about the "relationship" itself, not the targeted resources. For JSON API
   +   * with Drupal, relationship routes are like looking at an entity reference
   +   * field without loading the entities. It only reveals the type of the
   +   * targeted resource and the target resource IDs. These type+ID combos are
   +   * referred to as "resource identifiers."
   

   This is SUCH A GOOD EXPLANATION! We should have this in jsonapi.api.php, and do it for every non-trivial route ("individual" and "collection" seem trivial).

4. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -840,6 +846,255 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +  protected function getExpectedGetRelationshipDocument($relationship_field_name) {
   

   s/getExpectedGetRel…/getExpectedRel…/, no?

5. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -840,6 +846,255 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +  protected function getExpectedGetRelationshipDocumentData($relationship_field_name) {
   

   😍 — so elegant!

6. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -840,6 +846,255 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +        // Empty to-one relationships should be NULL and empty to-many
   +        // relationships should be an empty array.
   ...
   +            'data' => is_null($relationship_document['data']) ? NULL : [],
   

   These are strangely far apart.

7. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1801,108 +2048,101 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -  protected static function extractRelatedLinks(array $field_names, array $document) {
   -    return array_reduce($field_names, function ($links, $field_name) use ($document) {
   -      if ($link = array_reduce(
   -        ['data', 'relationships', $field_name, 'links', 'related'],
   -        'array_column',
   -        [$document]
   ...
   +  protected function getRelationshipFieldNames() {
   +    // Only content entity types can have relationships.
   +    $fields = $this->entity instanceof ContentEntityInterface
   +      ? iterator_to_array($this->entity)
   +      : [];
   +    return array_reduce($fields, function ($field_names, $field) {
   +      /* @var \Drupal\Core\Field\FieldItemListInterface $field */
   +      if ($this->isReferenceFieldDefinition($field->getFieldDefinition())) {
   

   Getting the list of relationship field names switched from introspecting JSON API responses to introspecting an entity in PHP. The latter is better for test coverage, because it would uncover anything that's consistently missing in JSON API. So: 👍

8. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1801,108 +2048,101 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -  protected function getRelatedResponses(array $links, array $request_options) {
   -    return array_reduce(array_keys($links), function ($related_responses, $field_name) use ($links, $request_options) {
   -      $related_response = $this->request('GET', Url::fromUri($links[$field_name]), $request_options);
   -      $cacheability = new CacheableMetadata();
   -      if ($cache_tags = $related_response->getHeader('X-Drupal-Cache-Tags')) {
   -        $cacheability->addCacheTags(explode(' ', $cache_tags[0]));
   -      }
   -      if ($cache_contexts = $related_response->getHeader('X-Drupal-Cache-Contexts')) {
   -        $cacheability->addCacheContexts(explode(' ', $cache_contexts[0]));
   -      }
   -      if ($dynamic_cache = $related_response->getHeader('X-Drupal-Dynamic-Cache')) {
   -        $cacheability->setCacheMaxAge(($dynamic_cache[0] === 'UNCACHEABLE') ? 0 : Cache::PERMANENT);
   -      }
   -      $related_document = Json::decode($related_response->getBody());
   -      $related_responses[$field_name] = (new ResourceResponse($related_document))
   -        ->addCacheableDependency($cacheability);
   -      return $related_responses;
   -    }, []);
   +  protected function getRelatedResponses(array $relationship_field_names, array $request_options) {
   +    $links = array_map(function ($relationship_field_name) {
   +      return static::getRelatedLink(static::toResourceIdentifier($this->entity), $relationship_field_name);
   +    }, array_combine($relationship_field_names, $relationship_field_names));
   +    return $this->getResponses($links, $request_options);
      }
   

   👏 — again, so elegant!

9. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1801,108 +2048,101 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +  protected function getRelationshipResponses(array $relationship_field_names, array $request_options) {
   ...
   +  protected function getResponses(array $links, array $request_options) {
   

   Don't these also belong on the ResourceResponseTestTrait?

#63:

4. Ok, let's keep this for now then, we can still change it anyway

+++ b/tests/src/Functional/BlockContentTypeTest.php
@@ -102,4 +102,11 @@ class BlockContentTypeTest extends ResourceTestBase {
+  public function testRelated() {
+    $this->markTestSkipped('Remove this in https://www.drupal.org/project/jsonapi/issues/2940339');
+  }

+++ b/tests/src/Functional/CommentTest.php
@@ -394,4 +394,11 @@ class CommentTest extends ResourceTestBase {
+  public function testRelated() {
+    $this->markTestSkipped('Remove this in https://www.drupal.org/project/jsonapi/issues/2940339');
+  }

+++ b/tests/src/Functional/UserTest.php
@@ -444,4 +444,11 @@ class UserTest extends ResourceTestBase {
+  public function testGetRelationships() {
+    $this->markTestSkipped('Remove this in https://www.drupal.org/project/jsonapi/issues/2952506');
+  }

Excellent! This actually also makes #2940339: Port reference field support for non-empty entity reference fields not pointing to an entity from #2543726 simpler, because simply removing these overrides then gives us the necessary test coverage there!

Since we added skipped tests and have linked issues, this is no longer blocked.

EXCELLENT!

There are still 2 CS violations… :P

Other than that, I think this looks great. Ideally, we get @e0ipso's +1. But because it doesn't change any logic, that's not a hard requirement I think?

At the very least, this is a huge step forward! :)

🎉

Gabe opened #2953318: Comprehensive JSON API integration test coverage phase 4: collections, filtering and sorting and #2953321: Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets.