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
- 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
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
Comment #2
bradjones1At first glance I like option 2 more, since it's also reducing often-repeated boilerplate.
Comment #3
bbralaUpdated 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 ;))
Comment #4
bbralaComment #5
bradjones1Sounds like a solid plan.
Comment #9
bbralaOk, tests are green. While moving the tests had a nice moment when I already got some better feedback ^^
I'll open a MR also with some test failures.
Comment #11
bbralaExample failure when passing in invalid langcode.
Comment #12
bbralaComment #14
bbralaThe testfailure is an unrelated failure in
Drupal.FunctionalJavascriptTests.Ajax.AjaxTest.Comment #17
bbralaComment #20
bradjones1A 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.
Comment #21
longwaveComment #22
bbralaWould 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.
Comment #23
bbralaComment #24
bbralaComment #25
bradjones1While I made a commit on this it was basically also a code-review so I will be bold and RTBC this.
Comment #26
quietone commentedIt took a while to look at the changes but I got there in the end. I left some questions in the MR.
Comment #27
bbralaGood 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.
Comment #28
quietone commentedThanks 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
Comment #29
pravesh_poonia commentedsorry, comment by mistake
Comment #30
acbramley commentedThis 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
Comment #31
bradjones1Comment #32
bradjones1Re: #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.
Comment #33
bbralaHuh, there was a second one? Might be mixing up two issues though. I'll fix it
Comment #34
bbralaScrewed up a rebase lol ;p
Anyways, all should be fine again.
Comment #35
bradjones1Rebase looks good, target changed. Back to RTBC.
Comment #36
larowlanThis feels like a quality of life task to me.
Comment #37
larowlanJust one comment on the MR, fine to self RTBC
Comment #38
larowlanUpdated issue summary to remove the redundant option 1.
Can we get a change record here please? Thanks!
Comment #39
bbralaMoved interface up, and added a change record describing the improvement.
Comment #40
bbralaHmmz, 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.
Comment #41
bbralaComment #42
bbralaComment #43
bbralaI'm gonna be bold and keep rtbc. Seems like a really really minor change.
Comment #47
quietone commentedI 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
Comment #48
bbralaAwesome, this will help a lot while developing jsonapi. :)
Comment #49
quietone commentedJust remembered to change the version.