Problem/Motivation

A lot of times when JSON:API test fail there is an error which is not communicated to the developer in a helpfull way. This makes contributing and debugging very hard. The error message when something is wrong a lot of the times is:

1) Drupal\Tests\jsonapi\Functional\ShortcutTest::testCollectionFilterAccess
Undefined array key "data"

This means the test assume $document['data'], but cannot find it. When inpecting the resulting document one can actually find the error in the response. eg:

Array
        (
            [title] => Forbidden
            [status] => 403
            [detail] => The current user is not authorized to filter by the `spotlight` field, given in the path `spotlight`.
            [links] => ...
        )

This is helpfull information on what went wrong.

Steps to reproduce

Run a test that erros, it will give the unhelpfull 'Undefined array key "data"' if a test asserts data but cannot find is.

Proposed resolution

Add a method for parsing the request body. A lot of our code looks like this $doc = Json::decode((string) $response->getBody());.

  /**
   * @param \Psr\Http\Message\ResponseInterface $response
   *   Response to extract JSON:API document from.
   * @param bool $dataRequired
   *   Validate the data property is available in the response.
   *
   * @return ?array
   *   JSON:API document extracted from the response.
   */
  protected function getDocumentFromResponse(ResponseInterface $response, bool $dataRequired = TRUE): ?array {
    assert($this instanceof BrowserTestBase);

    $document = Json::decode((string) $response->getBody());

    if ($dataRequired === TRUE && !isset($document['data'])) {
      if (isset($document['errors'])) {
        $errors = [];
        foreach ($document['errors'] as $error) {
          $errors[] = $error['title'] . ' (' . $error['status'] . '): ' . $error['detail'];
        }
        $this->fail('Missing expected data property in document. Error(s): ' . PHP_EOL . '  ' . implode('  ' . PHP_EOL, $errors));
      }
      $this->fail('Missing expected data property in document but no errors found. Response body: ' . PHP_EOL . '  ' . $response->getBody());
    }
    return $document;
  }

So this would replace:

$doc = Json::decode((string) $response->getBody());

With:

$doc = $this->getDocumentFromResponse($response);

If one expects errors you can turn the validation off. Another option would be to just parse the reponse normally. I considered allowing to specify what key it validated, but that seems overkill.

// Documents expecting a reponse that is not valid. Not by default since most calls require 
// the data key. Sometimes you want to validate errors, but that is less frequent.
$docWithErrors = $this->getDocumentFromResponse($response, FALSE);

Remaining tasks

  1. Add a change record

User interface changes

n.a.

API changes

Adds a new trait that is used in functional tests in the jsonapi module.

Data model changes

n.a.

Release notes snippet

JSON:API tests expecting a document with data now verify the existence on the data attribute and give usefull errors when they fail. use this in test by using the getDocumentFromResponse method in the GetDocumentFromRequestTrait.

Issue fork drupal-3440993

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

bbrala created an issue. See original summary.

bradjones1’s picture

At first glance I like option 2 more, since it's also reducing often-repeated boilerplate.

bbrala’s picture

Issue summary: View changes

Updated issue summary, changed default for second option to TRUE since most calls require a data key in the reponse. Sometimes you dont want to check errors, this would then mean setting the second argument to FALSE (or not using that method ;))

bbrala’s picture

Issue summary: View changes
bradjones1’s picture

Sounds like a solid plan.

bbrala’s picture

Ok, tests are green. While moving the tests had a nice moment when I already got some better feedback ^^

1) Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest::testWrite
    Missing expected data property in document. Errors: 
      Bad Request: Resource object must include a "type".

I'll open a MR also with some test failures.

bbrala’s picture

Status: Active » Needs review

Example failure when passing in invalid langcode.

Drupal\Tests\jsonapi\Functional\JsonApiFunctionalMultilingualTest::testPatchTranslation
Missing expected data property in document. Error(s): 
  Unprocessable Content: langcode.0: The value you selected is not a valid choice.

core/modules/jsonapi/tests/src/Traits/GetDocumentFromRequestTrait.php:33
core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalMultilingualTest.php:173
vendor/phpunit/phpunit/src/Framework/TestResult.php:729
bbrala’s picture

Issue summary: View changes

bbrala’s picture

The testfailure is an unrelated failure in Drupal.FunctionalJavascriptTests.Ajax.AjaxTest.

bbrala changed the visibility of the branch 3440993-jsonapi-test-failures to hidden.

bbrala changed the visibility of the branch 3440993-jsonapi-test-failures to active.

bbrala’s picture

Issue summary: View changes

bradjones1 changed the visibility of the branch 3440993-jsonapi-test-failures to hidden.

bradjones1 changed the visibility of the branch 3440993-jsonapi-test-failures to active.

bradjones1’s picture

Title: JSON:API test failures are very unhefpfull at times. » JSON:API test failures are very unhefpful at times.

A few nits and renamings, but I think this is a nice DX and test ergonomics improvement, and cuts down on code repetition. More maintainable, too.

longwave’s picture

Title: JSON:API test failures are very unhefpful at times. » JSON:API test failures are very unhelpful at times.
bbrala’s picture

Would be nice if we can RTBC this, got a committers eye on this, which could mean this could get in pretty quickly. Would love to use this feature soon to make development easier.

bbrala’s picture

Title: JSON:API test failures are very unhelpful at times. » Improve JSON:API test failure messages to include errors when data is expeceted
bbrala’s picture

Title: Improve JSON:API test failure messages to include errors when data is expeceted » Improve JSON:API test failure messages to include errors when data is expected
bradjones1’s picture

Status: Needs review » Reviewed & tested by the community

While I made a commit on this it was basically also a code-review so I will be bold and RTBC this.

quietone’s picture

Status: Reviewed & tested by the community » Needs work

It took a while to look at the changes but I got there in the end. I left some questions in the MR.

bbrala’s picture

Status: Needs work » Needs review

Good suggestions. I've changed the signature and updated the weird variable name.

In regards to the order of 200 vs getting the document, in my opinion it is important to change the order of those to get optimal value from this change.

quietone’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for pointing that out. It makes perfect sense to do the status check after.

The items I pointed out have been addressed, so restoring RTBC

pravesh_poonia’s picture

sorry, comment by mistake

acbramley’s picture

This is great! Anything to improve these tests is much appreciated, maybe we should turn #3395417: Tests extending jsonapi's ResourceTestBase are extremely hard to debug/maintain/deal with into a meta?

Does this not need to go into 11.x first? mr is against 10.3.x

bradjones1’s picture

Version: 10.3.x-dev » 11.x-dev
bradjones1’s picture

Status: Reviewed & tested by the community » Needs work

Re: #30 good catch, the MR applies clean against 11.x but it does need a target change. @bbrala, since you own that MR?

NW briefly for that, and to re-queue tests against HEAD.

bbrala’s picture

Huh, there was a second one? Might be mixing up two issues though. I'll fix it

bbrala’s picture

Status: Needs work » Needs review

Screwed up a rebase lol ;p

Anyways, all should be fine again.

bradjones1’s picture

Status: Needs review » Reviewed & tested by the community

Rebase looks good, target changed. Back to RTBC.

larowlan’s picture

Category: Feature request » Task

This feels like a quality of life task to me.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

Just one comment on the MR, fine to self RTBC

larowlan’s picture

Issue summary: View changes
Issue tags: +Needs change record

Updated issue summary to remove the redundant option 1.

Can we get a change record here please? Thanks!

bbrala’s picture

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

Moved interface up, and added a change record describing the improvement.

bbrala’s picture

Hmmz, I think this needs a last minor change.

$errors[] = $error['title'] . ': ' . $error['detail'];

Should be:

$errors[] = $error['title'] . ' (' . $error['status'] . '): ' . $error['detail'];

That will help in some cases.

bbrala’s picture

Issue summary: View changes
bbrala’s picture

Issue summary: View changes
bbrala’s picture

I'm gonna be bold and keep rtbc. Seems like a really really minor change.

  • quietone committed 0fe7080e on 10.3.x
    Issue #3440993 by bbrala, bradjones1, larowlan, quietone: Improve JSON:...

  • quietone committed f2fb38ac on 10.4.x
    Issue #3440993 by bbrala, bradjones1, larowlan, quietone: Improve JSON:...

  • quietone committed 74a6dce2 on 11.x
    Issue #3440993 by bbrala, bradjones1, larowlan, quietone: Improve JSON:...
quietone’s picture

Status: Reviewed & tested by the community » Fixed

I was about to commit this when I realized that there are no tests of the new trait. I then checked with the heuristics in the Testing Core Gate, Gong through the questions I concluded that this does not need tests. And I did force errors locally to confirm that it does indeed produce the improved messages. Still, I checked in committer Slack.

After chatting with larowlan we both agree that tests are not needed here. He also pointed out that manual testing was also done in #11, which I had forgotten. he would like to backport to 10.2.x and I don't think this meets the criteria in the 'allowed changes' doc.

Therefore, committed to 11.x, 10.4.x and 10.3.x

bbrala’s picture

Awesome, this will help a lot while developing jsonapi. :)

quietone’s picture

Version: 11.x-dev » 10.3.x-dev

Just remembered to change the version.

  • quietone committed 74a6dce2 on 11.0.x
    Issue #3440993 by bbrala, bradjones1, larowlan, quietone: Improve JSON:...

Status: Fixed » Closed (fixed)

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