Tests extending jsonapi's ResourceTestBase are extremely hard to debug/maintain/deal with

Created on 19 October 2023, 8 months ago
Updated 20 October 2023, 8 months ago

Problem/Motivation

Every time I (and some of my colleagues have echoed the same) try to work on an issue that requires changes to any tests extending from
Drupal\Tests\jsonapi\Functional\ResourceTestBase I find it very difficult to figure out what is going on and where something needs to be changed.

πŸ› Information disclosure access bypass for revision log fields when the JSON:API module is enabled Fixed is one such issue. Even with Xdebug, it's very difficult to figure out what should be happening at each step of the test, who's logged in, and what's actually being tested. This makes fixing the failures in those tests very hard, and in the past has completely put me off continuing with trying to get an issue over the line.

For example, it took me ~20 minutes to find out why BlockContentTest::testGetIndividual was failing in 3395404.

It was failing because the expected document wasn't correct: The 'data' member was not as expected. that's on line 805 of ResourceTestBase, with the following call stack:

 /data/app/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:805
 /data/app/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:724
 /data/app/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:988

Going back up to line 988 and working my way backwards, I can't see why a user would have access to view the revision log there. That requires with view revision, view all revisions, or update operation access based on the fix in that issue. When looking in the setUp it looks like the user under test has no roles and I confirmed that via debugging.

However, I missed this line $this->setUpAuthorization('GET');

In there we grant permissions directly to the authenticated user role.

To find that out, I had to debug right down into the BlockContentAccessControlHandler (after failing to find the right place to breakpoint several times in the test) and discovered the user had administer blocks why does a user have that when testing a view operation? Turns out setUpAuthorization grants that permission for GET methods.

Great! It seems like administer blocks really shouldn't be used for GET methods (access block library is the only permission required for viewing a block_content entity that's published), so let's only grant that permission for GET methods.

But GET methods cover testGetIndividual, testCollection, AND testRevisions. Those are 3 very different things, and the latter 2 do require the administer (or other more specific) permissions.

You can see where I'm going with this hopefully....every time I have to touch these tests it feels like I'm balancing 17 sticks with broken plates on top of them and trying to make everything work just right but it never seems to be easy meaning there's weird workarounds littered through the tests for special conditions, etc.

Proposed resolution

Decouple tests a lot, moving things out of ResourceTestBase into their own classes, making it easier to understand what's really being tested for each entity type.

Remaining tasks

Agree
Plan
Code

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component
JSON APIΒ  β†’

Last updated 2 days ago

Created by

πŸ‡¦πŸ‡ΊAustralia acbramley

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @acbramley
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Ok now I need to debug a failure in testCollection (meaning figuring out how getExpectedCollectionResponse works) and wow...I really feel for anyone that's not as familiar with Drupal trying to grok this.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    At the very least, when one of these tests fails, we should make it easy to see what's wrong with the expected vs actual document.

    This is unparseable, why are the attribute names stripped?

Production build 0.69.0 2024