Problem/Motivation

Time and time again, we keep running into bizarre problems that we are not supposed to run into. These problems prevent small, focused patches in the REST and serialization modules from being viable: in virtually every single REST issue, we encounter bizarre behavior and mind-bending bugs, which simply explode the original/intended issue scope out of pure necessity.

Also, all the existing test coverage is centered on the fairly artificial entity_test entity type. There are incomplete, superficial tests for some entity types, but they each test only a small subset, and every time they test it incompletely (e.g. they assert that the proper response code is sent, but they do not assert the response body). There's @todos in almost all existing tests to remind ourselves to add test coverage for other entity types. So, those things never got resolved before the 8.0.0 release.

Examples:

Proposed resolution

Steps:

  1. Refactor/rewrite all of CreateTest, ReadTest, UpdateTest, DeleteTest to provide an entire CRUD flow including testing of the edge cases to verify the DX (e.g. forgetting X-CSRF-Token header). Put all that in a base class, and then subclass it for every entity type. That is then much like \Drupal\system\Tests\Entity\EntityCacheTagsTestBase, which has been very successful. That then means any module providing a new entity type can just subclass that test and get full REST test coverage.
  2. Figure out a way how to test all serialization formats, including those added by additional modules. That means it's two-dimensional extensibility … which means it's kinda tricky to make that work. We'll see.
  3. Delete the old (Read|Create|Update|Delete)Test files. Deprecate their base class. But this too is fairly large, so to make reviews easier, it's probably better to delete the old, incomplete test coverage in a follow-up: #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase.
  4. Review/get to RTBC
  5. Commit
  6. Work on follow-ups: #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase + #2824572: [PP-11] Write EntityResourceTestBase subclasses for every other entity type..

Crucial background information for patch reviewers & core commiters

Probably the best proof/most convincing argument for having these improved tests is the fact that working on the patch for this issue uncovered more than 20 issues. That's >20 issues that get in the way while communicating with Drupal via APIs. Several of those 20 issues are not in the rest.module component, but in the routing system, request processing system and serialization.module components. Hence they also affect https://www.drupal.org/project/jsonapi — which we are trying to bring to Drupal core as an experimental module. So this test coverage being much more strict helps uncover problems we didn't know about, and problems we would certainly run into.

At ~190 KB, this is a very large patch. It's 100% test coverage. It's 100% additions. This does not modify any code, hence there's zero risk. There's ~70 KB of test code removal in #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase (which I felt would be better to do in a follow-up to make patch reviews easier, but I'd be totally fine with merging it into this patch if that's what reviewers/core committers prefer).

However, what is key, is that 90% of what needs to be reviewed lives in only two files: ResourceTestBase and EntityResourceTestBase.

ResourceTestBase
It extends BrowserTestBase because it does functional testing: it performs actual requests, just like front end developers would. The whole point is indeed that we test from the perspective of the (front end) developer who is trying to use the REST API. And in case they make a mistake in their request (i.e. it's not well-formed), the server should provide a helpful response. The test coverage must assert this helpful response.
Talking more concretely, a concrete subclass of ResourceTestBase must specify a static::$format (e.g. json, hal_json), a static::$mimeType (e.g. application/hal+json), a static::$expectedErrorMimeType (e.g. application/json) and a static::$auth. Its setUp() method ensures that a user for testing is created, and it also ensures the anonymous user and the authenticated user both have no permissions at all. This allows REST tests to test responses when the user does not have sufficient permissions. The only other noteworthy thing it provides is the provisionResource() method, which provisions a resource by creating the necessary REST resource config entity for a particular test.
The intent is for every REST resource plugin to provide its own abstract subclass of ResourceTestBase, testing all expected responses (behavior). It is then a simple matter of subclassing that class with a static::$format, static::$auth etc so that in a few lines, you have a concrete test for that particular REST resource plugin + format + authentication provider combination.
EntityResourceTestBase
This is the abstract base class of ResourceTestBase for the \Drupal\rest\Plugin\rest\resource\EntityResource REST resource plugin.
It has testGet(), testPost(), testPatch() and testDelete(). Each of those run through extensive scenarios with detailed assertions.
Because each entity type has different fields and its own peculiarities, there is again a subclass for every entity type: NodeResourceTestBase, BlockResourceTestBase, and so on.
This class then has dozens of subclasses, to test concrete format+auth combinations: NodeHalJsonAnonTest, NodeHalJsonCookieTest, NodeJsonBasicAuthTest, and so on.
Some of these subclasses of EntityResourceTestBase then have their own additional test methods, to test further special cases. For example: CommentResourceTestBase::testPostDxWithoutCriticalBaseFields() and UserResourceTestBase::testPatchDxForSecuritySensitiveBaseFields().

You may wonder Why can't this use data providers? — the answer is simple: these tests are designed to be usable/extensible by contrib modules. Contrib modules that:

  • provide new REST resource plugins can subclass ResourceTestBase
  • provide new entity types can subclass EntityResourceTestBase
  • provide new formats can subclass NodeResourceTestBase/BlockResourceTestBase/… and get strong assurances that their format behaves as expected for each of those entity types, with minimal effort. See for example \Drupal\Tests\hal\Functional\EntityResource\Term\TermHalJsonAnonTest.
  • provide new authentication providers can subclass NodeResourceTestBase/BlockResourceTestBase/… and get strong assurances that their format behaves as expected for each of those entity types

So we can't use data providers for:

  • REST resource plugins, because they behave differently
  • formats, because contrib might add more, and we could not possibly know what their special cases are — for example HAL+JSON moves reference fields to _links and _embedded, adds lang attributes to translatable fields, and so on. We couldn't possibly know those special cases for all formats
  • authentication providers, because contrib might add more, and we could not possibly know what their particular mechanisms are — for example Basic Auth with incorrect login info causes 403s to become 401s, and Cookie requires a CSRF token to be sent with every request

I hope this was useful!

Remaining tasks

Phase 1: infrastructure (base classes with scenarios)
  1. Port ReadTest
  2. Port CreateTest
  3. Port UpdateTest
  4. Port DeleteTest
Phase 1B: apply this to all formats & authentication providers
Formats: json, hal_json. (Not doing xml because #2800873: Add XML GET REST test coverage, work around XML encoder quirks.)
Authentication providers: anon, cookie, basic_auth.
Phase 2: apply this to all entity types
Doing all entity types in this issue would make this patch impossibly large. It was deemed wiser to only apply this new test coverage to all entity types that already had some test coverage in the existing REST tests: Node, User, Term, EntityTest, Comment, Block, ConfigTest, Role and Vocabulary.
All other entity types are deferred to a follow-up: #2824572: [PP-11] Write EntityResourceTestBase subclasses for every other entity type..
Content entity types:
  1. Node
  2. User
  3. MenuLinkContent
  4. BlockContent
  5. File
  6. Shortcut
  7. Item
  8. ContentModerationState
  9. Term
  10. EntityTest
  11. Feed
  12. Message
  13. Comment

Config entity types:

  1. Block
  2. FieldStorageConfig
  3. FilterFormat
  4. View
  5. SearchPage
  6. ConfigurableLanguage
  7. Action
  8. ContentLanguageSettings
  9. EntityFormDisplay
  10. EntityViewDisplay
  11. FieldConfig
  12. BaseFieldOverride
  13. ConfigTest
  14. Tour
  15. ResponsiveImageStyle
  16. RdfMapping
  17. Role
  18. DateFormat
  19. RestResourceConfig
  20. BlockContentType
  21. CommentType
  22. EntityTestBundle
  23. NodeType
  24. ContactForm
  25. ShortcutSet
  26. Vocabulary
  27. EntityFormMode
  28. EntityViewMode
  29. Editor
  30. Menu
  31. ModerationStateTransition
  32. ModerationState
  33. ImageStyle
Phase 3: refinements: entity type-specific additions, format-specific additions, authentication mechanism-specific additions
The existing edge case test coverage from (Read|Create|Update|Delete)Test should be ported and where possible/sensible, it should be generalized to all entity types.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#164 ultra_llama_mode_on.png91.26 KBvaplas
#162 finally.JPG208.76 KBWim Leers
#158 interdiff-8.3.x.txt3.87 KBWim Leers
#158 rest_comprehensive_tests-2737719-157--8.3.x.patch190.01 KBWim Leers
#158 rest_comprehensive_tests-2737719-156--8.2.x.patch188.73 KBWim Leers
#156 interdiff.txt3.5 KBWim Leers
#156 rest_comprehensive_tests-2737719-156--8.3.x.patch190.03 KBWim Leers
#156 rest_comprehensive_tests-2737719-156--8.2.x.patch188.73 KBWim Leers
#137 rest_comprehensive_tests-2737719-136--8.3.x.patch190.06 KBWim Leers
#137 interdiff-8.3.x.txt3.89 KBWim Leers
#137 rest_comprehensive_tests-2737719-129--8.2.x.patch188.76 KBWim Leers
#134 rest_comprehensive_tests-2737719-134--8.3.x.patch189.48 KBWim Leers
#134 interdiff-8.3.x.txt3.8 KBWim Leers
#134 rest_comprehensive_tests-2737719-129--8.2.x.patch188.76 KBWim Leers
#129 interdiff.txt22.25 KBWim Leers
#129 rest_comprehensive_tests-2737719-129.patch188.76 KBWim Leers
#128 phpcs.txt14.91 KBcatch
#121 interdiff.txt14.86 KBWim Leers
#121 rest_comprehensive_tests-2737719-121.patch188.95 KBWim Leers
#119 interdiff.txt5.37 KBWim Leers
#119 rest_comprehensive_tests-2737719-119.patch188.23 KBWim Leers
#117 interdiff.txt30.07 KBWim Leers
#117 rest_comprehensive_tests-2737719-117.patch187.3 KBWim Leers
#114 interdiff.txt8.24 KBWim Leers
#114 rest_comprehensive_tests-2737719-114.patch188.65 KBWim Leers
#113 interdiff.txt11.52 KBWim Leers
#113 rest_comprehensive_tests-2737719-113.patch183.57 KBWim Leers
#108 rest_comprehensive_tests-2737719-108.patch180.07 KBWim Leers
#108 interdiff.txt34.02 KBWim Leers
#106 rest_comprehensive_tests-2737719-104.patch174.43 KBWim Leers
#106 interdiff.txt24.98 KBWim Leers
#103 interdiff.txt6.5 KBWim Leers
#103 rest_comprehensive_tests-2737719-103.patch167.75 KBWim Leers
#102 rest_comprehensive_tests-2737719-102.patch162.08 KBWim Leers
#102 interdiff.txt1.76 KBWim Leers
#100 interdiff.txt2.84 KBWim Leers
#100 rest_comprehensive_tests-2737719-100.patch161.98 KBWim Leers
#97 interdiff.txt12.79 KBWim Leers
#97 rest_comprehensive_tests-2737719-97.patch160.72 KBWim Leers
#92 interdiff.txt23.39 KBWim Leers
#92 rest_comprehensive_tests-2737719-92.patch147.78 KBWim Leers
#91 interdiff.txt16.42 KBWim Leers
#91 rest_comprehensive_tests-2737719-91.patch147.78 KBWim Leers
#90 interdiff.txt2.09 KBWim Leers
#90 rest_comprehensive_tests-2737719-90.patch146.94 KBWim Leers
#89 interdiff.txt29.48 KBWim Leers
#89 rest_comprehensive_tests-2737719-89.patch146.79 KBWim Leers
#88 rest_comprehensive_tests-2737719-88.patch125.41 KBWim Leers
#88 interdiff.txt5.44 KBWim Leers
#87 interdiff.txt1.49 KBWim Leers
#87 rest_comprehensive_tests-2737719-87.patch122.6 KBWim Leers
#85 rest_comprehensive_tests-2737719-85.patch122.49 KBWim Leers
#85 interdiff.txt3.14 KBWim Leers
#84 interdiff.txt1.41 KBWim Leers
#84 rest_comprehensive_tests-2737719-84.patch121.4 KBWim Leers
#80 interdiff.txt1.91 KBWim Leers
#80 rest_comprehensive_tests-2737719-80.patch121.41 KBWim Leers
#77 rest_comprehensive_tests-2737719-77.patch121.21 KBWim Leers
#77 interdiff.txt13.46 KBWim Leers
#76 rest_comprehensive_tests-2737719-76.patch111.79 KBWim Leers
#76 interdiff.txt8.96 KBWim Leers
#75 rest_comprehensive_tests-2737719-75.patch111.78 KBWim Leers
#75 interdiff.txt5.04 KBWim Leers
#72 rest_comprehensive_tests-2737719-72.patch109.46 KBWim Leers
#72 interdiff.txt12.81 KBWim Leers
#71 rest_comprehensive_tests-2737719-71.patch96.71 KBWim Leers
#71 interdiff.txt2.3 KBWim Leers
#67 rest_comprehensive_tests-2737719-67.patch97 KBWim Leers
#67 interdiff.txt2.05 KBWim Leers
#67 interdiff.txt3.3 KBWim Leers
#66 interdiff.txt2.65 KBWim Leers
#66 rest_comprehensive_tests-2737719-66.patch97 KBWim Leers
#65 interdiff.txt5.47 KBWim Leers
#65 rest_comprehensive_tests-2737719-65.patch95.71 KBWim Leers
#63 rest_comprehensive_tests-2737719-63.patch92.92 KBWim Leers
#63 interdiff.txt8.63 KBWim Leers
#62 interdiff.txt1.02 KBWim Leers
#62 rest_comprehensive_tests-2737719-62.patch93.6 KBWim Leers
#60 interdiff.txt1.11 KBWim Leers
#60 rest_comprehensive_tests-2737719-60.patch93.67 KBWim Leers
#57 rest_comprehensive_tests-2737719-55.patch93.62 KBWim Leers
#55 rest_config_simplify-2721595-55.patch36.14 KBWim Leers
#55 interdiff.txt20.18 KBWim Leers
#52 rest_comprehensive_tests-2737719-52.patch85.53 KBWim Leers
#52 interdiff.txt42.39 KBWim Leers
#50 interdiff.txt9.72 KBWim Leers
#50 rest_comprehensive_tests-2737719-50.patch87.9 KBWim Leers
#49 rest_comprehensive_tests-2737719-49.patch78.29 KBWim Leers
#49 interdiff.txt7.87 KBWim Leers
#48 rest_comprehensive_tests-2737719-48.patch70.53 KBWim Leers
#48 interdiff.txt9.04 KBWim Leers
#47 rest_comprehensive_tests-2737719-47.patch61.57 KBWim Leers
#47 interdiff.txt11.18 KBWim Leers
#46 interdiff.txt1.07 KBWim Leers
#46 rest_comprehensive_tests-2737719-46.patch61.17 KBWim Leers
#41 rest_comprehensive_tests-2737719-41.patch61.06 KBWim Leers
#41 interdiff.txt48.74 KBWim Leers
#37 interdiff.txt1.57 KBWim Leers
#37 rest_comprehensive_tests-2737719-37.patch36.66 KBWim Leers
#32 interdiff.txt9.21 KBWim Leers
#32 rest_comprehensive_tests-2737719-32.patch36.26 KBWim Leers
#30 rest_comprehensive_tests-2737719-30.patch28.18 KBWim Leers
#30 interdiff.txt742 bytesWim Leers
#28 interdiff.txt10.19 KBWim Leers
#28 rest_comprehensive_tests-2737719-28.patch28.21 KBWim Leers
#27 interdiff.txt4.07 KBWim Leers
#27 rest_comprehensive_tests-2737719-27.patch22.41 KBWim Leers
#26 rest_comprehensive_tests-2737719-26.patch21.78 KBWim Leers
#26 interdiff.txt551 bytesWim Leers
#24 rest_comprehensive_tests-2737719-24.patch21.8 KBWim Leers
#24 interdiff.txt1.19 KBWim Leers
#22 rest_comprehensive_tests-2737719-22.patch22.93 KBWim Leers
#22 interdiff.txt1.66 KBWim Leers
#20 rest_comprehensive_tests-2737719-20.patch22.99 KBWim Leers
#20 interdiff.txt1.63 KBWim Leers
#18 rest_comprehensive_tests-2737719-18.patch22.9 KBWim Leers
#18 interdiff.txt1013 bytesWim Leers
#16 interdiff.txt3.78 KBWim Leers
#16 rest_comprehensive_tests-2737719-16.patch22.89 KBWim Leers
#13 interdiff.txt1.18 KBWim Leers
#13 rest_comprehensive_tests-2737719-13.patch22.86 KBWim Leers
#12 rest_comprehensive_tests-2737719-12--individual_commits.patch40.32 KBWim Leers
#12 rest_comprehensive_tests-2737719-12.patch21.72 KBWim Leers
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

larowlan’s picture

Bonus points: switch to BTB at same time :)

dawehner’s picture

Bonus points: switch to BTB at same time :)

There is also the conversion to guzzle which ensures we don't need this uggly hack any longer: #1841474: Convert REST tests to Guzzle (would also help with core moving from WTB to BTB)

andypost’s picture

Also would be great to extend test coverage when *page_cache* modules enabled for https://www.drupal.org/node/2541358/revisions/view/9428580/9735215

Wim Leers’s picture

#5: Sure, but that's of the least concern right now. Plus, there are no known bugs wrt REST + (Dynamic) Page Cache.

tedbow’s picture

Assigned: Unassigned » tedbow
Put all that in a base class, and then subclass it for every entity type. That is then much like \Drupal\system\Tests\Entity\EntityCacheTagsTestBase,

Does this that we would have 1 test class for every subclass of \Drupal\Core\Entity\Entity? So 30+ test files?

I am going to assign this to me to start working on it. @Wim Leers let me know if you have already started on it.

Wim Leers’s picture

I've not started on it. And yes, one for every entity type, but not the two dozen or so "test" entity types. So, probably about a dozen unique tests. But look at e.g. \Drupal\node\Tests\NodeCacheTagsTest + \Drupal\comment\Tests\CommentCacheTagsTest + \Drupal\user\Tests\UserCacheTagsTest + … — there's almost zero code in there :)

tedbow’s picture

@Wim Leers ok thanks for the clarification

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method » [PP-1] EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
Assigned: tedbow » Wim Leers
Status: Active » Postponed

This is blocked on #1841474: Convert REST tests to Guzzle (would also help with core moving from WTB to BTB). I'm going to be working on that issue, as well as this one in the coming days.

Wim Leers’s picture

Title: [PP-1] EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method » EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
Status: Postponed » Needs review
FileSize
21.72 KB
40.32 KB

While #1841474: Convert REST tests to Guzzle (would also help with core moving from WTB to BTB) is waiting to be committed, I've already begun working on this one. This will not be finished before that one lands anyway.


I've got a start here now. This is testing the Node entity type against multiple formats (hal_json + json, xml is omitted for now — see next comment), and for now only with a single authentication mechanism (basic_auth). They follow a shared, standardized test scenario, that is specified in EntityResourceTestBase — which is the REST equivalent for the comprehensive cache tags test coverage at \Drupal\system\Tests\Entity\EntityCacheTagsTestBase.

For now, this only testing GET: it includes a superset of the test scenario that \Drupal\rest\Tests\ReadTest uses.

(The individual-commits patch is included to show how I got to this point. From now on, I'll omit that, every interdiff will correspond to a local commit.)

Wim Leers’s picture

This is all that it takes to test an additional format. Pretty nice, is it not? :)

Except… this is something HEAD does not test. And so the very first thing I test that HEAD is not testing… is broken: the attached patch will fail. I opened an issue for it: #2800873: Add XML GET REST test coverage, work around XML encoder quirks.

The last submitted patch, 12: rest_comprehensive_tests-2737719-12.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: rest_comprehensive_tests-2737719-13.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
22.89 KB
3.78 KB

Apparently I've had the base URL configured incorrectly in my unit tests for a very long time. Hence wrong code, hence #12 failed. This fixes it. Now you'll see the 2 fails in #13 more clearly.

Status: Needs review » Needs work

The last submitted patch, 16: rest_comprehensive_tests-2737719-16.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1013 bytes
22.9 KB

I failed to include all my changes.

Status: Needs review » Needs work

The last submitted patch, 18: rest_comprehensive_tests-2737719-18.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
22.99 KB

New URL-related fail. This time for the error message that we asse
rt. Apparently $entity->toUrl() generates a relative URL that is relative to the Drupal installation, and not relative to the domain. This should solve that.

(I lifted the solution from the CDN module for Drupal 8.)

Status: Needs review » Needs work

The last submitted patch, 20: rest_comprehensive_tests-2737719-20.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
22.93 KB

I've been interpreting that error message incorrectly. Sorry for the noise. Fingers crossed.

Status: Needs review » Needs work

The last submitted patch, 22: rest_comprehensive_tests-2737719-22.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
21.8 KB

There we are! Finally.

fail: [Other] Line 81 of core/modules/rest/src/Tests/NodeXmlBasicAuthTest.php:
Drupal\rest\Tests\NodeXmlBasicAuthTest::testGet
Symfony\Component\Serializer\Exception\UnexpectedValueException: A string must be provided as a bundle value.

This is what I alluded to in #13, where I wrote:

Except… this is something HEAD does not test. And so the very first thing I test that HEAD is not testing… is broken: the attached patch will fail. I opened an issue for it: #2800873: Add XML GET REST test coverage, work around XML encoder quirks.

So, for now, reverting the XML test coverage. We don't have that in HEAD either. And because we don't, we didn't know this didn't work. This patch/test coverage is already learning us what does and doesn't work, and it's only in the early stages!

No need to be sad, we are learning where our flaws are :)

Status: Needs review » Needs work

The last submitted patch, 24: rest_comprehensive_tests-2737719-24.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
551 bytes
21.78 KB

Gah.

Wim Leers’s picture

Moving all the things to the right places.

Wim Leers’s picture

Now no longer testing only Node: added TermResourceTestBase, TermJsonBasicAuthTest and TermHalJsonBasicAuthTest.

Status: Needs review » Needs work

The last submitted patch, 28: rest_comprehensive_tests-2737719-28.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
742 bytes
28.18 KB

Same mistake as in NodeResourceTestBase, which I fixed in #26. Same fix.

dawehner’s picture

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,224 @@
    +      $message = ['message' => 'The "' . static::$entityType . '" parameter was not converted for the path "' . $path . '" (route name: "rest.entity.' . static::$entityType . '.GET.' . static::$format . '")'];
    

    Nitpick: Did you tried using " {...} " to make this maybe a bit more readable?

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,224 @@
    +      $encode_options = ['json_encode_options' => JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT];
    +      $this->assertSame($this->serializer->encode($message, static::$format, $encode_options), (string)$response->getBody());
    

    A bit nicer assertion: assertJsonStringEqualsJsonFile

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,224 @@
    +  public function atestPost() {
    

    Maybe use camelCase here? Otherwise it reads a lot like attest, which has a total different meaning

  4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,224 @@
    +    // Try as user with sufficient permissions.
    +    $this->performUnsafeOperation('POST');
    +    $this->assertSame(201, $this->getSession()->getStatusCode());
    +    $this->assertSession()->responseContains('…');
    +  }
    

    It is a bit weird that we are not using guzzle here

  5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    @@ -0,0 +1,88 @@
    +    assert('$this->entity instanceof \Drupal\Core\Entity\EntityInterface', 'Entity must already have been created.');
    

    If you care about that, why not use $this->assertInstanceOf?

Wim Leers’s picture

@dawehner: Thanks for the review! Will address in the next round.


While working on porting over user_role testing from HEAD's ReadTest, I made a mistake (I forgot to enable the hal module for a test that was testing hal_json, which caused its GET route to not exist). This caused me to run into another bug: the serializer.formats container parameter is used to create REST GET routes. So even if you only allow hal_json, it'll still create GET routes for json and xml (i.e. the default formats).

I predicted this bug almost 4 months ago at #2737751: [PP-2] Refactor REST routing to address its brittleness:

  1. ResourceBase generates individual GET routes for every available serialization format, not just the ones that are allowed by the configuration, and then ResourceRoutes::alter() removes those that are not acceptable by the configuration.

Attached is a reroll that adds test coverage for user_role and block config entities.

So, the patch currently ports a superset of the test logic in ReadTest, for node, taxonomy_term, block and user_role. term wasn't tested before. That leaves entity_test, config_test and taxonomy_vocabulary to be ported to have all of the test coverage in ReadTest.

Now we're really getting up to speed :)

Wim Leers’s picture

Issue summary: View changes

Updated the issue summary to list all the currently known/planned todos.

Wim Leers’s picture

Version: 8.3.x-dev » 8.2.x-dev

Since this is about providing test coverage to ensure that REST in Drupal 8.2 is as reliable as possible, this really should go in 8.2. This won't cause API changes. At most, it will cause bug fixes. Which is the whole point.

Wim Leers’s picture

Issue summary: View changes

Marking the already completed items as completed in the IS.

Status: Needs review » Needs work

The last submitted patch, 32: rest_comprehensive_tests-2737719-32.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
36.66 KB
1.57 KB

I had it ready, but I failed to include a small change in #32 to correctly generate URLs for config entities. Here's the fix.

Status: Needs review » Needs work

The last submitted patch, 37: rest_comprehensive_tests-2737719-37.patch, failed testing.

almaudoh’s picture

This looks like a huuuge scope to squeeze into one issue. Can this be split up into smaller issues (maybe phases 2 & 3 after phase 1 is done).

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
36.76 KB

I have no idea what's going on in #37, because all those tests pass fine locally. It's the same assertion that (only) testbot keeps failing on in every scenario, but only when using basic_auth: instead of Drupal returning text/html; charset=UTF-8, it's returning text/plain; charset=UTF-8. No idea how that is possible.

For now, commenting that assertion, along with a @todo comment. Let's hope there's not another assertion further in the testGet() scenario that also fails…

Wim Leers’s picture

So far, I've labeled all tests as "basic auth". But actually, in reality, they're testing the behavior in case of no authentication, i.e. the anonymous case. Given sufficient permissions to the anonymous user, this is also possible. Which makes sense, because some REST APIs are intended for public (authless) consumption.

After quite a struggle, I got that working!

In this reroll, every existing test is split up in a "basic auth" test and an "anon" test. Any contrib module that provides a new authentication mechanism can then provide similar, simple test classes with barely any code, and hence run the entire test suite!


The struggle is because of the weird interaction/design of the routing + authentication system, which causes things to fail miserably in an unfriendly way when the user forgets to specify ?_format=nonsense, or has a typo (for example ?_format=haljson on a URL that already has a HTML route.

It's very very very confusing how Symfony's routing system first grants access because a user is authenticated via basic auth, and so AccessAwareRouter says everything is okay, and then afterwards, during AuthenticationSubscriber::onKenrelRequestFilterProvider(), we choose to deny access after all because basic_auth isn't a globally allowed access provider, and the matched route (which matches the HTML, non-REST route due to the missing ?format=) is lacking an _auth route option. You can either get a 406 (which is what we want), or a 403 with HTML (in case of non-specified or invalid format) or a 403 in the expected format (in case of non-specified or invalid format plus Accept header, even though we don't actually support Accept headers… thanks to DefaultExceptionSubscriber calling Request::getAcceptableContentTypes()).
Worse, it's even possible to get the appropriate 406 response in case of an invalid format, but for that response to be sent with Content-Type: text/html! (In case of anonymous.)

More edge cases than I can count.

Seems like the authentication system and routing system are integrated in a brittle, confusing, backward manner. And the content negotiation system makes it that tiny bit extra unpredictable. This is no one's fault. This is complex software with (too) many layers, and without comprehensive integration tests, this sort of thing is to be expected.

So, the updated test coverage verifies this crazy behavior is at least consistent. And then when we fix it, in #2805279: Routing system + authentication system + format-specific routes (e.g. those in rest.module) = frustrating, unhelpful 403 responses instead of 406 responses, we can massively simplify this crazy test logic. It's better to embrace and accept the status quo, because that means we are better prepared to improve it :)

Finally, less important, but still a WTF: the error response body in case of missing Basic Authentication is different for the json and hal_json formats. In the former case, it's actually in JSON, but with a strange prefix, and in the latter case, it's in plain text… opened an issue for that too: #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json.

Finally^2 (I found this just before posting), I found another absolutely fascinating edge case. I'll let the comment in the test speak for itself:

    // All blocks can be viewed by the anonymous user by default. An interesting
    // side effect of this is that any anonymous user is also able to read the
    // corresponding block config entity via REST, even if an authentication
    // provider is configured for the block config entity REST resource! In
    // other words: Block entities do not distinguish between 'view' as in
    // "render on a page" and 'view' as in "read the configuration".
    // This prevents that.
    // @todo Investigate further.
    …
Wim Leers’s picture

Issue tags: +blocker

I just postponed #2135829: EntityResource: translations support on this — without this issue, that issue is very likely to introduce even more edge cases.

Wim Leers’s picture

#39: Yes, the vast majority of phase 2 will definitely be handled by other issues. But as the issue summary says, this must provide at least parity with (i.e. a superset of) the existing tests. Which will then allow us to remove those tests.

Status: Needs review » Needs work

The last submitted patch, 41: rest_comprehensive_tests-2737719-41.patch, failed testing.

Wim Leers’s picture

Quoting myself in #40:

I have no idea what's going on in #37, because all those tests pass fine locally. It's the same assertion that (only) testbot keeps failing on in every scenario, but only when using basic_auth: instead of Drupal returning text/html; charset=UTF-8, it's returning text/plain; charset=UTF-8. No idea how that is possible.

For now, commenting that assertion, along with a @todo comment. Let's hope there's not another assertion further in the testGet() scenario that also fails…

Clearly, my fear came true. Sigh :(

What's interesting, is that 100% of the fails are for the hal_json tests. The tests for the json format are unaffected. Even more interesting: I observed in #41 that the 401 error response for the hal_json format marks itself as Content-Type: application/hal+json, yet the response body is plain text. Funny enough… on testbot it apparently does say Content-Type: text/plain, which is consistent with the response body!

I can't reproduce this when

  1. using Drupal in the root of the domain (http://tres/) and
    1. running the test directly with PHPUnit
    2. running all --group hal tests with PHPUnit
    3. running the test directly with run-tests.sh
    4. running all --module hal tests with run-tests.sh
  2. using Drupal in a subdirectory of a domain (http://localhost/tres/ and
    1. running the test directly with PHPUnit
    2. running all --group hal tests with PHPUnit
    3. running the test directly with run-tests.sh
    4. running all --module hal tests with run-tests.sh

So I'm completely out of ideas. I'm going to have to comment out all the Content-Type response header assertions. We'll need to sort this out before this eventually gets committed. I officially give up.

The final idea I had is that perhaps the response contains two Content-Type headers, and that somehow testbot parses the first one and my local installation parses the second one (or vice versa). But, no such luck:

HTTP/1.1 401 Unauthorized
Date: Mon, 26 Sep 2016 15:58:03 GMT
Server: Apache/2.2.29 (Unix) DAV/2 PHP/5.5.11 mod_ssl/2.2.29 OpenSSL/0.9.8zg
X-Content-Type-Options: nosniff
X-Powered-By: PHP/5.5.11
WWW-Authenticate: Basic realm="tres"
Cache-Control: must-revalidate, no-cache, private
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Content-Length: 39
Connection: close
Content-Type: application/hal+json

No authentication credentials provided.
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
61.17 KB
1.07 KB
Wim Leers’s picture

So now that we have a decent starting point, moving the test coverage to the modules responsible for the biggest divergence, and hence the biggest reason for them to have explicit comprehensive test coverage: those providing additional formats.

In this reroll, I moved all hal_json tests (filenames matching *HalJson*Test.php) to the hal module. The https://www.drupal.org/project/jsonapi module will be able to basically copy/paste and rename these.

Wim Leers’s picture

Added entity_test content entity test coverage. (Because ReadTest::testRead() is testing this, so we need it for test coverage parity.)

Wim Leers’s picture

Whew! After a >24 hour detour on new bugs, here I am again with another update!

Added taxonomy_vocabulary config entity test coverage.

Discovered another problem: #2807263: Impossible to write unit tests involving Vocabulary, because TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE) are defined in taxonomy.module instead of VocabularyInterface. Which uncovered yet another problem, which also happens to improve anonymous REST response performance significantly: #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty.

So, the anonymous vocabulary tests have their GET test coverage disabled for now, until #2807263: Impossible to write unit tests involving Vocabulary, because TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE) are defined in taxonomy.module instead of VocabularyInterface lands.

Wim Leers’s picture

Added config_test config entity test coverage.

This didn't uncover new bugs — hurray! But some special extra sauce was necessary to make config_test properly testable. See the tiny config_test_rest test-only module for why.

This marks the completion of porting 100% of the test coverage of ReadTest over to this new test coverage! It now not only does a superset of the test scenario, it now also tests all the entity types that ReadTest was testing.

Next: CreateTest.

Wim Leers’s picture

The experience I gained with working on Vocabulary-related things for this issue, and the fact that I got the HTTP GET tests for it to pass in #49 allowed me to answer #2772413: REST GET fails on entity/taxonomy_vocabulary/{id} 403 Forbidden with error with confidence. The problem must've been somewhere else.

And sure enough, the problem turns out to have been that they did not grant the administer taxonomy permission. But of course, doing that would also be quite dangerous, if all you need is the ability to read vocabularies!

So, we need to solve that on two levels:

  1. saner default access to Vocabulary entities: #2808217: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission (patch posted!)
  2. better error responses, that tell you why you're getting a 403: #2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out

Those are the two newest child issues for this issue!

Wim Leers’s picture

I've got the POST test coverage working (ported most of CreateTest). It took a lot of effort, and I uncovered lots of bugs. But, before getting to that point, and posting that interdiff+patch, I'm posting some refactoring that was necessary to get there. That'll make the POST test coverage easier to review.

  1. Rename setUpAuthentication() to setUpAuthorization(): this was not handling authentication, but authorization: I named it poorly when I introduced it.
  2. Rather than letting setUpAuthorization() create accounts, but only in case of non-anon tests, I moved that logic to ResourceTestBase. Less code in actual tests, so less that can go wrong. Now it only needs to grant permissions to either the anonymous or authenticated role.
  3. Added ResourceTestBase::assertResourceResponse().
  4. Renamed+moved EntityResourceTestBase::assertErrorResponse() to ResourceTestBase::assertResourceErrorResponse().
  5. Added ResourceTestBase::grantPermissionsToTestedRole(), which calls grantPermissionsToAuthenticatedRole() when an authentication provider is being used, and calls grantPermissionsToAnonymousRole() otherwise. This allows us to remove the setUpAuthorization() method in every single concrete test class, and move it in the per-entity type base classes. So much simpler! This helps reduce patch size too :)
dawehner’s picture

To be honest, in order to be able to review that patch, but pushing everything into one patch makes it really hard to do so.

To be honest, this really reads like a good usecase for a dataprovider.

Wim Leers’s picture

Discussed with dawehner in IRC.

  1. This patch only needs very high-level review for now: test infrastructure, plus the scenarios (in EntityResourceTestBase).
  2. I'm working towards that!
Wim Leers’s picture

And here it is! The test of all the fundamental things in CreateTest (which tests POSTing of entities). This is not yet a superset of CreateTest.

Necessary to achieve superset:

  1. the BC flag (also need to add this for GET test coverage)
  2. field-level validation errors
  3. X-CSRF-Token header testing (but this requires the cookie authentication provider, which I've yet to add test coverage for)
  4. Comment entity type
  5. User entity type

Already surpassing existing test coverage in that it tests Term and has far stricter assertions, hence stronger guarantees.


While working on that, I encountered no less than seven bugs/WTFs that resulted in new issues, and which slowed me down on the order of many days:

  1. A confusing error message when forgetting to send the Content-Type request header: #2811133: Confusing error response set by ContentTypeHeaderMatcher when forgetting the Content-Type request header
  2. #2817727: Add test coverage to prove controller is called *after* authentication validation
  3. Inconsistently encoded JSON responses: #2813755: JSON responses encoded inconsistently: make them all RFC4627-compliant
  4. Inconsistent error responses: #2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers
  5. Manual route rebuilding required: #2815845: Importing (deploying) REST resource config entities should automatically do the necessary route rebuilding
  6. Authentication WTF wrt anon: #2817737: A REST resource's "auth" configuration MUST be non-empty, which prevents configuring it for anon access only
  7. Authentication WTF wrt cookies: #2817745: Add test coverage to prove that REST resource's "auth" configuration is also not allowing global authentication providers like "cookie" when not listed

Status: Needs review » Needs work

The last submitted patch, 55: rest_config_simplify-2721595-55.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
93.62 KB

I simply uploaded the wrong patch. They were listed just above each other, I ended up selecting the wrong one, because my computer is super laggy.

Status: Needs review » Needs work

The last submitted patch, 57: rest_comprehensive_tests-2737719-55.patch, failed testing.

almaudoh’s picture

I simply uploaded the wrong patch.

I was wondering about the sudden change in patch size...

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
93.67 KB
1.11 KB

Status: Needs review » Needs work

The last submitted patch, 60: rest_comprehensive_tests-2737719-60.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
93.6 KB
1.02 KB

24 different failures this time. Failed because #2811133: Confusing error response set by ContentTypeHeaderMatcher when forgetting the Content-Type request header landed while I was working on this, which means one @todo in this patch can be fixed :)

Wim Leers’s picture

Update testGet() to match the improvements in testPost().

Wim Leers’s picture

Issue summary: View changes

Fixing mistake in IS: there's no test coverage yet for using the cookie authentication provider.

Wim Leers’s picture

Test what happens before provisioning a REST resource and before granting the necessary authorizations. Both for testGet() and testPost().

Wim Leers’s picture

Test BC permissions. Both for testGet() and testPost(). One less @todo!.

Wim Leers’s picture

#65 contains a flaw that will cause failures for basic_auth tests (it will only work for anon===authless tests).

The last submitted patch, 65: rest_comprehensive_tests-2737719-65.patch, failed testing.

The last submitted patch, 66: rest_comprehensive_tests-2737719-66.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 67: rest_comprehensive_tests-2737719-67.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.3 KB
96.71 KB

Apparently another fail sneaked in there. In all Block config entities. The root cause is that same problem that was discovered before (see #41):

    // All blocks can be viewed by the anonymous user by default. An interesting
    // side effect of this is that any anonymous user is also able to read the
    // corresponding block config entity via REST, even if an authentication
    // provider is configured for the block config entity REST resource! In
    // other words: Block entities do not distinguish between 'view' as in
    // "render on a page" and 'view' as in "read the configuration".

But #65 introduced additional assertions to ensure a 403 is returned before the necessary authorization is set up. The work-around I had in place didn't account for that. So, now I have changed the work-around to disallow access by any user until the necessary permission is granted. We still need to fix the root cause though.

Created an issue for that now: #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity".

Wim Leers’s picture

Test coverage for the Comment entity, for parity with CreateTest.

klausi’s picture

  1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Vocabulary/VocabularyHalJsonAnonTest.php
    @@ -0,0 +1,39 @@
    +  // Disable the GET test coverage due to bug in taxonomy module.
    +  // @todo Fix in https://www.drupal.org/node/2805281: remove this override.
    +  public function testGet() {}
    

    This test should be marked as skipped with $this->markTestSkipped().

    And the doc block should use /** ... */ comments.

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Block/BlockResourceTestBase.php
    @@ -0,0 +1,96 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function createEntity() {
    

    In Drupal when we use the terminology "create entity" then we mean create an entity object but do not save it. So I would name this "insertEntity()" or "createAndSaveEntity()" or similar.

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Block/BlockResourceTestBase.php
    @@ -0,0 +1,96 @@
    +    assert('$this->entity instanceof \Drupal\Core\Entity\EntityInterface', 'Entity must already have been created.');
    

    use $this->assertInstanceOf() instead.

  4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
    @@ -0,0 +1,224 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function createEntity() {
    +//    // Create a "Camelids" node type.
    +//    NodeType::create([
    +//      'name' => 'Camelids',
    +//      'type' => 'camelids',
    +//    ])->save();
    +//
    +//    // Create a "Llama" node.
    +//    $node = Node::create(['type' => 'camelids']);
    

    You are also setting up node types and whatever is necessary to actually save one entity. Should this be named "setupEntity()"?

  5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ConfigTest/ConfigTestResourceTestBase.php
    @@ -0,0 +1,61 @@
    +    $config_test = entity_create('config_test', [
    

    entity_create() is deprecated, let's not introduce more of it. Use the class instead.

  6. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
    @@ -0,0 +1,306 @@
    +    assert('[] === $user_role->getPermissions()', 'The anonymous user role has no permissions at all.');
    

    Should use $this->assertSame().

  7. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
    @@ -0,0 +1,306 @@
    +    // @todo Remove this in https://www.drupal.org/node/2815845.
    +    drupal_flush_all_caches();
    

    This is quite an expensive operation you do in quite a few places. Can you create a helper method that just clears the config cache and routing cache? That's probably what the @todo issue is about :)

I'm a bit concerned about test runtime. What is the runtime of all old REST tests vs. all the new Functional tests you introduced? Would be interesting to just get a feeling of how much we are adding here.

Can't say much about the overall approach with all the sub-classing, the patch is a bit too large for my taste. I would argue to do this in smaller chunks, but whatever works for you.

Wim Leers’s picture

#73:

  1. Thanks, will do!
  2. I'm just adopting the same approach/pattern as \Drupal\system\Tests\Entity\EntityCacheTagsTestBase::createEntity(). This is simply about creating an entity … and if it needs additional setup, then that too. I think "create entity" captures the spirit/intent best.
  3. No, this is an assert(), not a test.
  4. See 2.
  5. You don't want to know how hard I tried not to use this ;) But because somebody decided it was a good idea to have two different ConfigTest entity types, with the exact same name, that is sadly impossible.
  6. See 3.
  7. No, that's not at all what the @todo is about. See the issue that it mentions: #2815845: Importing (deploying) REST resource config entities should automatically do the necessary route rebuilding. Once that issue is fixed, these drupal_flush_all_caches() calls can simply be removed. The only reason they're there is because after hours of debugging, that was the only solution that worked. They were introduced in #55.

I'm a bit concerned about test runtime. What is the runtime of all old REST tests vs. all the new Functional tests you introduced? Would be interesting to just get a feeling of how much we are adding here.

The old REST tests will be removed. Test run time is less important than developers (both core developers and front end developers actually using D8's REST API) wasting countless hours on bizarre errors. This test suite helps prevent that. And considering how many new issues I've already opened because of the test coverage written so far, it's definitely helping.

Can't say much about the overall approach with all the sub-classing,

I agree that the subclassing approach is clunky/verbose. But it's the only way that works: this is how we can test every entity type + format + authentication provider combination, and allow contrib modules that add more of either of those three to get the same solid test coverage. (It also means these tests can easily be run in parallel btw!)

the patch is a bit too large for my taste. I would argue to do this in smaller chunks, but whatever works for you.

+1000! I wish I could keep it smaller. But… this is a necessary evil when you're developing one generic test with scenarios that exercise all edge cases (EntityResourceTestBase's testGet() + testPost()). You need to have a sufficient variety of entity types to test against, because if you start with only a single one, then you'll need to keep modifying that base class in every issue where you add a new entity type, hence resulting in many conflicting patches, but also a less cohesive EntityResourceTestBase base class.

Note that I'm only including those entity types that are already covered in the existing REST test coverage. All others I'll defer to follow-ups!


I will address #73.1!

Wim Leers’s picture

#72 added test coverage for Comment, but did not yet include test coverage for the insane edge cases/DX problems I encountered.

Opened an issue for that too: #2820364: Entity + Field validation constraints are processed in the incorrect order.

Wim Leers’s picture

Clean-up patch, that addresses #73.1 too. All of dawehner's feedback in #31 has already been addressed while posting all those intermediary steps.

Wim Leers’s picture

Test coverage for the User entity, for parity with CreateTest.

Wim Leers’s picture

Issue summary: View changes

The last submitted patch, 75: rest_comprehensive_tests-2737719-75.patch, failed testing.

Wim Leers’s picture

#75 apparently had some issues. This should fix some of those fails. The ones that say Failed asserting that false matches expected true. are a bit problematic: I can't reproduce those locally. So I added additional debug output.

The last submitted patch, 76: rest_comprehensive_tests-2737719-76.patch, failed testing.

The last submitted patch, 77: rest_comprehensive_tests-2737719-77.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 80: rest_comprehensive_tests-2737719-80.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
121.4 KB
1.41 KB

Okay… so this is yet another case of "DrupalCI has result X, locally it's result Y, and there's no way to debug DrupalCI". So I have to give up, and just make the assertions less strict.

Wim Leers’s picture

CreateTest was asserting (for a few entity types) that a too long UUID resulted in a 404. This now does that for all entity types.

Status: Needs review » Needs work

The last submitted patch, 85: rest_comprehensive_tests-2737719-85.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
122.6 KB
1.49 KB

#85 introduced a failure because HAL's FieldNormalizer is very unforgiving when you use {field_name: 'abc'}: you must always specify {field_name: [{value: 'abc'}]}.

I opened #2820743: FieldNormalizers are very unforgiving, impossible to debug error response given for fixing that.

Wim Leers’s picture

\Drupal\rest\Tests\CreateTest::testCreateEntityTest() was testing an access-protected configured field: a field that cannot be edited should result in a 403 and a response body with a meaningful message.

This brings that to EntityResourceTestBase and verifies this works for all entity types!

Wim Leers’s picture

This adds test coverage for using the cookie authentication provider. It also adds the X-CSRF-Token header missing/invalid edge case DX testing to the POST test case scenario.

While working on this, I discovered a bug in the "login" RPC endpoint: #2820888: Cookie authentication: the user.login.http route never supports the 'hal_json' format or some other format, depending on module order.

This was the final thing to achieve superset of CreateTest, which means as of this patch, we have a superset of CreateTest's test coverage! Marking that one as done in the IS.

Next: UpdateTest + DeleteTest.

Wim Leers’s picture

Previous patches introduced a bit of cruft, cleaning that up.

Wim Leers’s picture

While working on testPatch() (for porting UpdateTest), I noticed that getNormalizedEntityToCreate() was a bit of an unfortunate name. This renames it to getNormalizedPostEntity(). Which then opens the door for getNormalizedPatchEntity().

Wim Leers’s picture

Again while working on testPatch() (for porting UpdateTest), I noticed that verifyResponseWhenMissingAuthentication() was misnamed, it should have been named assertResponseWhenMissingAuthentication().

Wim Leers’s picture

This ports the majority of UpdateTest. Some edge cases in UpdateTest still need to be brought over and generalized (particularly those for Comments, i.e. the coverage added in #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it). But in most respects, this is already testing a vast superset of what HEAD's test coverage does.

No new bugs found for once!

Bugs were found, I just forgot about that over the weekend. They are:

  1. #2821013: EntityResource's field validation 403 message is inconsistent between POST and PATCH
  2. #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors
Wim Leers’s picture

This ports all of DeleteTest and in fact covers a superset already! :)

One more bug found, this time not in Drupal but in Symfony PHP! #2821711: Apache always sets Content-Type: text/html, even for DELETE requests

dawehner’s picture

  1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
    @@ -0,0 +1,56 @@
    +    if (!$this->entity instanceof FieldableEntityInterface) {
    +      throw new \LogicException('This trait should only be used for fieldable entity types.');
    +    }
    

    I'm wondering when we should use exceptions vs. assertions.

  2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
    @@ -0,0 +1,56 @@
    +      if ($field->getName() === $bundle_key) {
    +        return FALSE;
    +      }
    +      return $field instanceof EntityReferenceFieldItemListInterface;
    

    I think this one filter could be in one line

  3. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
    @@ -0,0 +1,56 @@
    +    foreach ($normalization as $field_name => $data) {
    +      if ($this->entity->$field_name->isEmpty()) {
    +        unset($normalization[$field_name]);
    +      }
    +    }
    

    This could be an array_filter as well

  4. +++ b/core/modules/hal/tests/src/Functional/HalJsonBasicAuthWorkaroundFor2805281Trait.php
    @@ -0,0 +1,24 @@
    +    // @todo this works fine locally, but on testbot it comes back with 'text/plain; charset=UTF-8'. WTF.
    +//    $this->assertSame(['application/hal+json'], $response->getHeader('Content-Type'));
    +    $this->assertSame('No authentication credentials provided.', (string)$response->getBody());
    

    I'm happy to investigate that

  5. +++ b/core/modules/rest/tests/src/Functional/AnonResourceTestTrait.php
    @@ -0,0 +1,22 @@
    +  protected function assertResponseWhenMissingAuthentication(ResponseInterface $response) {
    +    throw new \LogicException('When testing for anonymous users, authentication cannot be missing.');
    +  }
    

    should that maybe be abstract?

  6. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Block/BlockResourceTestBase.php
    @@ -0,0 +1,104 @@
    +        $this->entity->setVisibilityConfig('user_role', [])->save();
    

    One small trick I've beeing applying recently is that I typehint properties in subclasses, when you have more knowldge about them. In this case you could typehint the entity with the block configuration entity, so this method call is known. I'm just making a general comment here, feel free to ignore that.

  7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
    @@ -0,0 +1,251 @@
    +    // @todo Remove the try/catch in favor of the two commented lines in https://www.drupal.org/node/2820364.
    +    try {
    +      $response = $this->request('POST', $url, $request_options);
    +      // This happens on DrupalCI.
    +      $this->assertSame(500, $response->getStatusCode());
    +    }
    +    catch (\Exception $e) {
    +      // This happens on Wim's local machine.
    +      $this->assertSame("Error: Call to a member function get() on null\nDrupal\\comment\\Plugin\\Validation\\Constraint\\CommentNameConstraintValidator->getAnonymousContactDetailsSetting()() (Line: 96)\n", $e->getMessage());
    +    }
    

    wow

  8. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,567 @@
    +abstract class EntityResourceTestBase extends ResourceTestBase {
    

    I would like to review that more clearly, but I don't have the physical and mental power for that right now

  9. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,567 @@
    +  public function testGet() {
    ...
    +  public function testPost() {
    

    I'm wondering whether you could split this up into methods, so the code is a bit easier to read:

    $this->doX();
    $this->doY();
    $this->doZ();
    
  10. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
    @@ -0,0 +1,341 @@
    +/**
    + * Subclass this for every REST resource, every format and every auth mechanism.
    + */
    +abstract class ResourceTestBase extends BrowserTestBase {
    +
    

    Should we make the other ResourceTestBase deprecated?

  11. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
    @@ -0,0 +1,341 @@
    +  protected function request($method, Url $url, array $request_options) {
    +    try {
    +      $response = $this->httpClient->request($method, $url->toString(), $request_options);
    +    }
    +    catch (ClientException $e) {
    +      $response = $e->getResponse();
    +    }
    +    catch (ServerException $e) {
    +      $response = $e->getResponse();
    +    }
    +    return $response;
    +  }
    

    You can also just use $request_options['http_errors'] = FALSE and get the exact same output

Wim Leers’s picture

Thanks, will address these tomorrow! :)

Wim Leers’s picture

#95:

  1. You're right, I was using it inconsistently. You already commented on this in #31.5. I removed all the assertions in the getExpectedNormalizedEntity() implementations. I only kept two assertions: those in \Drupal\Tests\rest\Functional\ResourceTestBase::setUp(). I think it makes sense to use assert() there because it's asserting that the test environment itself is being set up correctly.
  2. Done.
  3. Done.
  4. Oh, that'd be splendid! Except that it's probably a waste of your time to try to make a temporary work-around be less hacky — your time would be better spent fixing #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json, which would remove that trait altogether :)
  5. It is an abstract method on ResourceTestBase :)
  6. +1, done!
  7. Indeed.
  8. Of course, I'd love to have your review of this! This is indeed the absolutely essential part of this patch. It has a generic test scenario for GET, POST, PATCH and DELETE which is applied to every entity type. So that's how you need to look at this: as test scenarios that expose both a correct request and common mistakes. In case of a mistake, we verify that the response provides a helpful message.
  9. I do not see how we can do that without making the scenarios impossible to follow. That's the whole point: each of those test methods is testing an entire scenario. It's already separated by HTTP method (GET/POST/PATCH/DELETE). Each of those scenarios follow the same pattern: start out without a provisioned REST resource, observe it fail, then provision, observe it fail due to lack of authorization, then grant authorization, then observe it fail due to incorrect request, etc. It always ends with a successful request.
    That being said, if you see a way to make this clearer without breaking up a scenario, then I'm all for it!
  10. Yes, IMO we should deprecate \Drupal\rest\Tests\RESTTestBase.
  11. :O I WISH I HAD KNOWN THIS! Thanks, improved that :)

Thanks again for your review! :) This patch is already better for it. Looking forward to your review of EntityResourceTestBase.

Wim Leers’s picture

Issue summary: View changes

Per #94, DeleteTest was fully ported. Updating IS to reflect that.

Per #93, UpdateTest was mostly ported: all of the generic stuff was ported, but now there's still the specialized test coverage that needs to be ported (for example that added by #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it). I'm working on generalizing it.

Once that is done, I consider this issue done. I will file follow-ups to add test coverage for all other entity types. And then I'd like to add a test that ensures that every entity type has this test coverage at least for core's formats+auth providers, so that one can be certain that every entity type can in fact be interacted with via REST. (Look at \Drupal\Tests\help\Kernel\HelpEmptyPageTest::testEmptyHookHelp() for inspiration on how to do that.)

Wim Leers’s picture

Issue summary: View changes

Of course, the cookie authentication provider's test coverage was also already completed, back in #89.

Wim Leers’s picture

So, the 3 things that still need to be ported:

  1. \Drupal\rest\Tests\UpdateTest::testPatchUpdate()'s Make sure that the field does not get deleted if it is not present in the PATCH request. portion exists in the new test coverage (the rest of it is already done, and more)
  2. \Drupal\rest\Tests\UpdateTest::testUpdateUser()'s test coverage that asserts some fields can only be modified when given the current password.
  3. \Drupal\rest\Tests\UpdateTest::testUpdateComment()'s test coverage for read-only fields (which BTW differs between HAL+JSON vs JSON…), which was introduced at #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it.

This reroll addresses the first one. In doing so, I stumbled upon two problems:

  1. new: #2822611: Document why UserInterface + FileInterface + MenuLinkContentInterface + … extend \Drupal\Core\Entity\ContentEntityInterface — turns out this is by design…
  2. pre-existing: #2808335-13: Changed time not updated when only non-translatable fields are changed on a translated entity, work-around included
Wim Leers’s picture

Status: Needs review » Needs work

DELETED (pasted my comment intended for another issue)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
162.08 KB
Wim Leers’s picture

FileSize
167.75 KB
6.5 KB

This addresses #100.2:

  1. \Drupal\rest\Tests\UpdateTest::testUpdateUser()'s test coverage that asserts some fields can only be modified when given the current password.

I ran into #2813755: JSON responses encoded inconsistently: make them all RFC4627-compliant once again while working on this one.

tstoeckler’s picture

There is no "Beverage transferral approved" value in the Status field on d.o but I guess this comment will suffice. Maybe I should open an issue in the webmasters queue... ;-)

Wim Leers’s picture

#104: :D

Wim Leers’s picture

And this addresses #100.3:

  1. \Drupal\rest\Tests\UpdateTest::testUpdateComment()'s test coverage for read-only fields (which BTW differs between HAL+JSON vs JSON…), which was introduced at #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it.

Doing so resurfaced the existing problems that were already documented in #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it. But thanks to the test comprehensive test coverage introduced here, it is much clearer/scarier now how the HAL normalization causes aberrant behavior. So, opened #2824271: HAL causes 'pid' and 'homepage' fields on Comment entity to _not_ be forbidden to send in PATCH requests to fix that.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs followup

A round of self-review.

  1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
    @@ -0,0 +1,73 @@
    +trait HalEntityNormalizationTrait {
    

    Needs docs.

  2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
    @@ -0,0 +1,73 @@
    +  protected function applyHalFieldNormalization(array $normalization) {
    

    Needs docs.

  3. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Vocabulary/VocabularyHalJsonAnonTest.php
    @@ -0,0 +1,43 @@
    +   * Disable the GET test coverage due to bug in taxonomy module.
    +   * @todo Fix in https://www.drupal.org/node/2805281: remove this override.
    

    Merge these comments.

  4. +++ b/core/modules/rest/tests/modules/rest_test/rest_test.module
    @@ -22,3 +22,24 @@ function rest_test_rest_relation_uri_alter(&$uri, $context = array()) {
    \ No newline at end of file
    

    Fix this.

  5. +++ b/core/modules/rest/tests/src/Functional/AnonResourceTestTrait.php
    @@ -0,0 +1,22 @@
    +trait AnonResourceTestTrait {
    

    Add docs.

  6. +++ b/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php
    @@ -0,0 +1,36 @@
    +/**
    + * ResourceTestBase::getAuthenticationRequestOptions() for basic_auth.
    + */
    +trait BasicAuthResourceTestTrait {
    +
    

    Improve docs.

  7. +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php
    @@ -0,0 +1,115 @@
    +/**
    + * ResourceTestBase::getAuthenticationRequestOptions() for cookie.
    + */
    +trait CookieResourceTestTrait {
    

    Again.

  8. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
    @@ -0,0 +1,295 @@
    +  public function testPostDxWithoutCriticalBaseFields() {
    

    Needs docs.

  9. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ConfigTest/ConfigTestResourceTestBase.php
    @@ -0,0 +1,72 @@
    +    $config_test = entity_create('config_test', [
    

    Document why this does not use ConfigTest::create().

  10. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +abstract class EntityResourceTestBase extends ResourceTestBase {
    

    Add @see examples to these docs.

  11. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +  protected static $entityType = NULL;
    

    s/$entityType/$entityTypeId/

  12. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +  protected static $patchProtectedFields;
    

    s/Fields/FieldNames/

  13. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +  protected static $labelField = NULL;
    

    s/Field/FieldName/

  14. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +  abstract protected function createEntity();
    ...
    +  abstract protected function getExpectedNormalizedEntity();
    ...
    +  abstract protected function getNormalizedPostEntity();
    

    Mention these in the class docblock.

  15. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +  public function testGet() {
    ...
    +  public function testPost() {
    ...
    +  public function testPatch() {
    ...
    +  public function testDelete() {
    

    Add docblocks to each of these, and ensure there's no other methods listed in between these.

  16. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestJsonBasicAuthTest.php
    @@ -0,0 +1,46 @@
    +  }
    +
    +
    +}
    

    Two newlines, should be one.

  17. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php
    @@ -0,0 +1,127 @@
    +    $entity_test = EntityTest::create(array(
    

    s/array()/[]/

  18. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    @@ -0,0 +1,140 @@
    +        // @todo Create issue similar to https://www.drupal.org/node/2808217.
    +        $this->grantPermissionsToTestedRole(['administer taxonomy']);
    

    Actually create this issue.

  19. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
    @@ -0,0 +1,336 @@
    +/**
    + * Subclass this for every REST resource, every format and every auth mechanism.
    + */
    +abstract class ResourceTestBase extends BrowserTestBase {
    +
    

    The docblock is both wrong and incomplete. Rewrite.

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

First round of issue summary update: to reflect the current status and scope. Scope-related follow-up created: #2824572: [PP-11] Write EntityResourceTestBase subclasses for every other entity type..

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs followup

Second round of issue summary update: to reflect what to do with the old tests. Related follow-up created: #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase.

Now all necessary follow-ups have been created.

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Third and final round of issue summary update. Added more context for reviewers & committers.

Wim Leers’s picture

Issue summary: View changes

Fix typo.

Wim Leers’s picture

While working on #2824576, I found one more thing that makes sense to assert: more cacheability aspects. The cache tags + contexts for the GET/HEAD response. And the absence of the X-Drupal-Cache header for unsafe methods (POST/PATCH/DELETE).

See #2824576-2: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase, point 4.

This adds those.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue summary: View changes
FileSize
188.65 KB
8.24 KB

And also while working on #2824576, there is one last thing I found that makes sense to add to the test coverage here. I missed it because we have NodeTest which duplicates big portions of (Read|Create|Update|Delete)Test. But the portions it doesn't duplicate, is for asserting the error response when the developer forgets to specify the bundle for the entity (or specifies an incorrect one). This is a common, easy mistake, and it's very much worth testing that this works correctly for all entity types, to ensure a good DX/RX.

See #2824576-2: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase, point 3.

This adds that test coverage. In doing so, I generalized it, so that it doesn't run just for Node entities using the json format, but for all entity types (that have bundles) and for all formats. This led me to discover that the hal normalization actually breaks ungracefully when you specify an invalid bundle (it fails gracefully when omitting a bundle completely): with a PHP error and without useful DX feedback. Opened #2824827: \Drupal\hal\Normalizer\ContentEntityNormalizer::denormalize() fails with fatal PHP error when bundles are missing from link relation types for that.

And with that, this is 100% ready for final review.

Wim Leers’s picture

Tagging with RX & DX, since this test coverage will help prevent regressions in RX & DX.

borisson_’s picture

I didn't get trough the entire patch on my lunchbreak, but I did find some small things. Almost all of them are docs related.

  1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonAnonTest.php
    @@ -0,0 +1,34 @@
    +  /**
    +   * {@inheritdoc}
    +   *
    +   * Anononymous users cannot edit their own comments.
    

    I don't think is allowed, to have both an inheritdoc and actual documentation in the same docblock, so the docs should be copied over.

  2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php
    @@ -0,0 +1,142 @@
    +abstract class CommentHalJsonTestBase extends CommentResourceTestBase  {
    

    Needs docblock

  3. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php
    @@ -0,0 +1,142 @@
    +   * {@inheritdoc}
    +   *
    +   * The HAL+JSON format causes different PATCH-protected fields. For some
    

    See above.

  4. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
    @@ -0,0 +1,128 @@
    + * Trait for EntityResourceTestBase subclasses testing formats using HAL normalization
    

    80 cols.

  5. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
    @@ -0,0 +1,128 @@
    +   * @param array $normalization
    +   *   An entity normalization.
    ...
    +  protected function applyHalFieldNormalization(array $normalization) {
    

    Can we document that this array has to be keyed by field name?

  6. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
    @@ -0,0 +1,128 @@
    +    $reference_fields = array_keys(array_filter($this->entity->getFields(), function (FieldItemListInterface $field) use ($bundle_key) {
    +      return ($field->getName() === $bundle_key) ? FALSE : $field instanceof EntityReferenceFieldItemListInterface;
    +    }));
    +    foreach ($reference_fields as $field_name) {
    +      unset($normalization[$field_name]);
    +    }
    

    I think we could remove the second foreach by wrapping this in additional array_filter(. But that wouldn't help with readability I think. So nevermind.

  7. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
    @@ -0,0 +1,128 @@
    +      // @todo use this commented line instead of the 3 lines thereafter once https://www.drupal.org/node/2813853 lands.
    ...
    +      // @todo use this commented line instead of the 3 lines thereafter once https://www.drupal.org/node/2813853 lands.
    

    80 cols

  8. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php
    @@ -0,0 +1,137 @@
    +    return Cache::mergeTags(parent::getExpectedCacheContexts(), ['url.site']);
    

    /s/mergeTags/mergeContexts

  9. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Term/TermHalJsonAnonTest.php
    @@ -0,0 +1,79 @@
    +    return  $normalization + [
    

    double space

  10. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Term/TermHalJsonAnonTest.php
    @@ -0,0 +1,79 @@
    +    return Cache::mergeTags(parent::getExpectedCacheContexts(), ['url.site']);
    

    /s/mergeTags/mergeContexts

  11. +++ b/core/modules/hal/tests/src/Functional/EntityResource/User/UserHalJsonAnonTest.php
    @@ -0,0 +1,79 @@
    +    return  $normalization + [
    

    Double space

  12. +++ b/core/modules/hal/tests/src/Functional/EntityResource/User/UserHalJsonAnonTest.php
    @@ -0,0 +1,79 @@
    +    return Cache::mergeTags(parent::getExpectedCacheContexts(), ['url.site']);
    

    /s/mergeTags/mergeContexts/

  13. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Vocabulary/VocabularyHalJsonAnonTest.php
    @@ -0,0 +1,42 @@
    +   * @todo Remove this override once https://www.drupal.org/node/2805281 is fixed.
    

    80 cols.

  14. +++ b/core/modules/hal/tests/src/Functional/HalJsonBasicAuthWorkaroundFor2805281Trait.php
    @@ -0,0 +1,24 @@
    +   * {@inheritdoc}
    ...
    +   * Note how the response claims it contains a application/hal+json body, but
    

    docs + inheritdoc

  15. +++ b/core/modules/hal/tests/src/Functional/HalJsonBasicAuthWorkaroundFor2805281Trait.php
    @@ -0,0 +1,24 @@
    +    // @todo this works fine locally, but on testbot it comes back with 'text/plain; charset=UTF-8'. WTF.
    +//    $this->assertSame(['application/hal+json'], $response->getHeader('Content-Type'));
    

    80 cols

  16. +++ b/core/modules/rest/tests/modules/rest_test/rest_test.module
    @@ -1,9 +1,9 @@
    -/**
    - * @file
    - * Contains hook implementations for testing REST module.
    - */
    

    I think the @file block here needs to stay, we can only remove them for files that contain one namespaced thing (class/interface/...)

  17. +++ b/core/modules/rest/tests/modules/rest_test/rest_test.module
    @@ -22,3 +22,24 @@ function rest_test_rest_relation_uri_alter(&$uri, $context = array()) {
    +        // Never ever allow this field to be viewed: this lets EntityResourceTestBase::testGet() test in a "vanilla" way.
    

    80 cols

  18. +++ b/core/modules/rest/tests/modules/rest_test/rest_test.module
    @@ -22,3 +22,24 @@ function rest_test_rest_relation_uri_alter(&$uri, $context = array()) {
    \ No newline at end of file
    

    Missing newline

  19. +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php
    @@ -0,0 +1,126 @@
    +    // @todo Remove the hardcoded use of the 'json' format, and use static::$format + static::$mimeType instead, once https://www.drupal.org/node/2820888 is fixed.
    

    80 cols.

  20. +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php
    @@ -0,0 +1,126 @@
    +
    +
    ...
    +
    +
    ...
    +
    +
    ...
    +
    +
    ...
    +
    +
    

    double blank lines

  21. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Block/BlockResourceTestBase.php
    @@ -0,0 +1,130 @@
    +    // @todo Investigate further.
    

    Let's create an issue and link to it?

  22. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Block/BlockResourceTestBase.php
    @@ -0,0 +1,130 @@
    +    // @todo Update when POSTing config entity support is added in https://www.drupal.org/node/2300677.
    

    80 cols

  23. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
    @@ -0,0 +1,307 @@
    +   * - base fields that are not marked as required in \Drupal\comment\Entity\Comment::baseFieldDefinitions()
    +   *   yet in fact are required.
    

    80 cols

  24. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
    @@ -0,0 +1,307 @@
    +    // @todo Remove the first line in favor of the commented line in https://www.drupal.org/node/2820364.
    ...
    +    // @todo Remove the try/catch in favor of the two commented lines in https://www.drupal.org/node/2820364.
    

    80 cols

  25. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ConfigTest/ConfigTestResourceTestBase.php
    @@ -0,0 +1,73 @@
    +    // @todo Update when POSTing config entity support is added in https://www.drupal.org/node/2300677.
    

    80 cols

  26. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1053 @@
    +      // @todo remove this if-test and its containing code when https://www.drupal.org/node/2808335 is fixed.
    

    80 cols

  27. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1053 @@
    +   * Tests GETting an entity, plus edge cases to ensure good DX.
    

    I had to read this 3 times before I figured out that GETting was not misspelled. Can we rewrite this to Tests a GET request for an entity, ...

  28. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
    @@ -0,0 +1,358 @@
    + * For more guidance see \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase
    + * which has recommendations for testing the \Drupal\rest\Plugin\rest\resource\EntityResource
    ...
    +   * @todo what about edge cases when multiple formats are enabled, e.g. Accepting one format, but sending with a different Content-Type?
    ...
    +   * @todo it SHOULD be possible to iterate over all authentication mechanisms and do all of those in a single test? The problem is that we'd then have to enable all modules that provide auth mechanisms. Which can include contrib. So doing a separate test per auth mechanism makes it easier for contrib to add tests.
    

    80 cols

Wim Leers’s picture

  1. There's lots of these already, see \Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator::getDefinition(), \Drupal\Core\Access\AccessResult::isAllowed, \Drupal\Core\Datetime\Element\Datelist::valueCallback(), and so on. 290 occurrences of the regex \* \{\@inheritdoc\}\n \*\n \*
  2. I don't think I can add a meaningful comment to this. Just like there are no comment docblocks on NodeResourceTestBase etc, because it's simply overkill/unnecessary.
  3. See 1.
  4. Fixed!
  5. Well… that's just an entity normalization. Normalization arrays always have meaningful keys.
  6. Heh :)
  7. Fixed!
  8. Oh, great catch, thanks, fixed!
  9. Fixed!
  10. Fixed!
  11. Fixed!
  12. Fixed!
  13. Fixed!
  14. See 1.
  15. Fixed!
  16. Another great catch — fixed!
  17. Fixed!
  18. Fixed!
  19. Fixed!
  20. This is intentional, for legibility. You'll see the same style in EntityResourceTestBase::test(Get|Post|Patch|Delete)(). And you'll see that it's very necessary there.
  21. Ah, yes, of course! I missed this one. Fun fact: I already created this issue two weeks ago, just forgot to update this comment :) #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity"
  22. Fixed!
  23. Fixed!
  24. Fixed!
  25. Fixed!
  26. Fixed!
  27. Done, and also updated test(Post|Patch|Delete)()
  28. Fixed!
Wim Leers’s picture

Final round of self-review & clean-up:

  1. went over every single remaining @todo to ensure they're correct. Two didn't have issues. One of those two I was able to remove+address. And the other I was able to simplify and link to an issue I had created prior (#2813755: JSON responses encoded inconsistently: make them all RFC4627-compliant).
  2. I added a @todo or #118.1, in the place that made me discover it.
  3. I added test coverage for #118.3.
  4. Fixed one inconsistency.
dawehner’s picture

  1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonAnonTest.php
    @@ -0,0 +1,34 @@
    +   *
    +   * Anononymous users cannot edit their own comments.
    +   *
    +   * @see \Drupal\comment\CommentAccessControlHandler::checkAccess
    +   *
    +   * Therefore we grant them the 'administer comments' permission for the
    +   * purpose of this test.
    +   *
    +   * @see ::setUpAuthorization
    +   */
    +  protected static $patchProtectedFieldNames = [
    +    'changed',
    +    'thread',
    +    'entity_type',
    +    'field_name',
    +    'entity_id',
    +  ];
    

    this comment is a bit weird. It seems to explain something different, than what is done below (setting some protected field names)

  2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php
    @@ -0,0 +1,142 @@
    +    $author = User::load($this->entity->getOwnerId());
    

    Is there a reason we don't use $this->entity->getOwner()?

  3. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
    @@ -0,0 +1,128 @@
    +      return ($field->getName() === $bundle_key) ? FALSE : $field instanceof EntityReferenceFieldItemListInterface;
    

    Nitpick: I think you could rewrite this to ($field->getName() !== $bundle_key) && $field instanceof EntityReferenceFieldItemListInterface.

    Here I my quick nodes:

    a ? FALSE : b
    
    
      a 0 1
    b
    
    0  0  0
    
    1  1  0
    
    (a !== $bundle_key) && $field instanceof ...
    
  4. +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php
    @@ -0,0 +1,127 @@
    +
    +    // Parse and store the session cookie.
    +    $this->sessionCookie = explode(';', $response->getHeader('Set-Cookie')[0], 2)[0];
    +
    ...
    +    $request_options[RequestOptions::HEADERS]['Cookie'] = $this->sessionCookie;
    

    I'm wondering whether you could get rid of this custom cookie handling by specifing a location for a cookie jar, see \GuzzleHttp\Middleware::cookies

  5. +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php
    @@ -0,0 +1,127 @@
    +    if (!in_array($method, ['HEAD', 'GET'])) {
    ...
    +    // X-CSRF-Token request header is unnecessary for safe HTTP methods. No need
    +    // for additional assertions.
    +    if (in_array($method, ['HEAD', 'GET'])) {
    

    Should we include 'OPTIONS' here as well?

  6. +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php
    @@ -0,0 +1,127 @@
    +
    +    unset($request_options[RequestOptions::HEADERS]['X-CSRF-Token']);
    +
    +
    +    // DX: 403 when missing X-CSRF-Token request header.
    ...
    +    $request_options[RequestOptions::HEADERS]['X-CSRF-Token'] = 'this-is-not-the-token-you-are-looking-for';
    +
    +
    +    // DX: 403 when invalid X-CSRF-Token request header.
    

    Feel free to ignore: It feels more readable to move this unset below the line of documentation, so you know what this line of code is doing.

  7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    + * If there is an entity type-specific edge case scenario to test, then add that
    + * to the entity type-specific abstract subclass. Example:
    + * \Drupal\Tests\rest\Functional\EntityResource\Comment\CommentResourceTestBase::testPostDxWithoutCriticalBaseFields
    + *
    + * If there is an entity type-specific format-specific edge case to test, then
    + * add that to a concrete subclass. Example:
    + * \Drupal\Tests\hal\Functional\EntityResource\Comment\CommentHalJsonTestBase::$patchProtectedFieldNames
    + */
    +abstract class EntityResourceTestBase extends ResourceTestBase {
    

    Epic documentation!

  8. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +    // DX: 404 when resource not provisioned, 403 if canonical route.
    +    $response = $this->request('GET', $url, $request_options);
    +    $this->assertSame($has_canonical_url ? 403 : 404, $response->getStatusCode());
    +    $this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type'));
    +
    ...
    +    // DX: 404 when resource not provisioned, 403 if canonical route.
    

    These two cases have the same documentation even if the first one mostly deals with the case of having no request format. I guess we could document that even without a request method, we need the resource to be provisioned.

  9. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +    $request_options = array_merge_recursive($request_options, $this->getAuthenticationRequestOptions('GET'));
    

    array_merge_recursive had some issues, which is why we have NesedArray::merge. Should we maybe switch to that one, just to not run into the same issues as well?

  10. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +    $this->assertEquals($this->getExpectedCacheTags(), empty($cache_tags_header_value) ? [] : explode(' ', $cache_tags_header_value));
    ...
    +    $this->assertEquals($this->getExpectedCacheContexts(), empty($cache_contexts_header_value) ? [] : explode(' ', $cache_contexts_header_value));
    

    Do you need this explicit empty check? Isn't explode(' ', '') === []

  11. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +    // Verify that the GET and HEAD responses are the same, that the only
    +    // difference is that there's no body.
    

    That sounds like weird english. What about using: 'are the same; The only different is that there's no body'

  12. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +    $request_options[RequestOptions::HEADERS]['Accept'] = static::$mimeType;
    +
    +
    +    // DX: 406 when requesting unsupported format but specifying Accept header.
    

    Here is also one of these cases where moving down the line would improve readability IMHO.

  13. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +      $this->assertTrue(FALSE !== strpos($response->getBody()->getContents(), htmlspecialchars('No "Content-Type" request header specified')));
    

    So we could use assertContains here?

  14. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +    $request_options = array_merge_recursive($request_options, $this->getAuthenticationRequestOptions('POST'));
    

    Another array_merge_recursive instance ... I mostly care about custom/contrib code also implementing these test classes.

  15. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +
    +    // DX: 422 when invalid entity: UUID field too long.
    +    $response = $this->request('POST', $url, $request_options);
    +    // @todo Uncomment, remove next 3 in https://www.drupal.org/node/2813755.
    +//    $this->assertErrorResponse(422, "Unprocessable Entity: validation failed.\nuuid.0.value: <em class=\"placeholder\">UUID</em>: may not be longer than 128 characters.\n", $response);
    +    $this->assertSame(422, $response->getStatusCode());
    +    $this->assertSame([static::$mimeType], $response->getHeader('Content-Type'));
    +    $this->assertSame($this->serializer->encode(['message' => "Unprocessable Entity: validation failed.\nuuid.0.value: <em class=\"placeholder\">UUID</em>: may not be longer than 128 characters.\n"], static::$format), (string) $response->getBody());
    

    I really like these kind of detailed test coverage

  16. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +    if ($entity_type->hasKey('bundle') && $entity_type->getBundleEntityType()) {
    

    can't something have bundles but no related bundle entity types? These could still valid usecases to test

  17. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +      // DX: 400 when incorrect entity type bundle is specified.
    +      $response = $this->request($method, $url, $request_options);
    +      // @todo use this commented line instead of the 3 lines thereafter once https://www.drupal.org/node/2813853 lands.
    +//      $this->assertResourceErrorResponse(400, '"bad_bundle_name" is not a valid bundle type for denormalization.', $response);
    +      $this->assertSame(400, $response->getStatusCode());
    +      $this->assertSame([static::$mimeType], $response->getHeader('Content-Type'));
    +      $this->assertSame($this->serializer->encode(['error' => '"bad_bundle_name" is not a valid bundle type for denormalization.'], static::$format), (string) $response->getBody());
    

    Honestly, I'd have somehow expected a 422 here

  18. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,1082 @@
    +   * Asserts a 406 response… or in some cases a 403 response, because weirdness.
    

    hahaha

  19. +++ b/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php
    @@ -0,0 +1,232 @@
    +
    +    // Verify that we can log in with the new password.
    +    $request_body = [
    +      'name' => $user->getAccountName(),
    +      'pass' => $new_password,
    +    ];
    +    $request_options = [
    +      RequestOptions::HEADERS => [],
    +      RequestOptions::BODY => $this->serializer->encode($request_body, 'json'),
    +    ];
    +    $response = $this->httpClient->request('POST', Url::fromRoute('user.login.http')->setRouteParameter('_format', 'json')->toString(), $request_options);
    +    $this->assertSame(200, $response->getStatusCode());
    +  }
    

    Nice!

  20. +++ b/core/modules/rest/tests/src/Functional/JsonBasicAuthWorkaroundFor2805281Trait.php
    @@ -0,0 +1,25 @@
    +    $this->assertSame('{"message":"A fatal error occurred: No authentication credentials provided."}', (string)$response->getBody());
    

    Nitpick ^10: There should be a empty space after the casting, so (string) $response->getBody()

Wim Leers’s picture

  1. You're right, that comment is weird. Fixed.
  2. Yes! This was very tricky to figure out, but using getOwner() meant we were calling \Drupal\user\Entity\User::getAnonymousUser(), which generates an in-memory User entity. Which has no UUID set, which meant that the code further down that sets the expected _links normalization would fail, because it expects ['uuid' => ['value' => NULL]], which is of course wrong. See \Drupal\comment\Entity\Comment::getOwner(). Documented this now.
  3. Nice catch, done.
  4. I considered this, but I like this approach, because it makes the requirements for an authentication provider very explicit. You can currently easily compare \Drupal\Tests\rest\Functional\CookieResourceTestTrait::getAuthenticationRequestOptions() with \Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait, and see what exactly is expected of each request. Contrib modules like simple_oauth will add their own, and it'll again be clear to compare. If we start using Guzzle's built-in "cookie tracking", then we lose that explicitness, that clarity.
  5. Good point. We should also allow TRACE then. See https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html. Note this is exactly what Django does too: https://docs.djangoproject.com/en/dev/ref/csrf/#how-it-works.
  6. Feeling free to ignore; I've done it this way for consistency with EntityResourceTestBase::test(Get|Post|Patch|Delete)()
  7. :) Glad you like it!
  8. Great catch, improved.
  9. Makes sense to me. Done.
  10. Yes, it's needed. explode(' ', '') === [''], sadly.
  11. Agreed, good find. Fixed.
  12. Again feeling free to ignore, same reason as point 6.
  13. Oh, yes, lovely!
  14. Yep, also fixed.
  15. :) I think it's a requirement for something that's the actual interface, the actual API. How else would we know that we break BC?
  16. If that's the case, then \Drupal\serialization\Normalizer\EntityNormalizer::denormalize() is also wrong, and we'd need to fix that. (In a separate issue of course, since this one is test-only.) Note how it does:
        $entity_type_definition = $this->entityManager->getDefinition($entity_type_id, FALSE);
        …
        if ($entity_type_definition->hasKey('bundle')) {
          …
          $bundle_type_id = $entity_type_definition->getBundleEntityType();
          $bundle_types = $bundle_type_id ? $this->entityManager->getStorage($bundle_type_id)->getQuery()->execute() : [];
          …
          // Make sure a bundle has been provided.
          if (!is_string($data[$bundle_key])) {
            throw new UnexpectedValueException('A string must be provided as a bundle value.');
          }
    
          // Make sure the submitted bundle is a valid bundle for the entity type.
          if ($bundle_types && !in_array($data[$bundle_key], $bundle_types)) {
            throw new UnexpectedValueException(sprintf('"%s" is not a valid bundle type for denormalization.', $data[$bundle_key]));
          }
    

    (i.e. any entity type definition that has a bundle key can have that bundle type unexpected error response occur, as long as there is a bundle entity type)
    … oh but wait, it's still possible to get the A string must be provided as a bundle value error response even when there is no bundle entity type! So you're partially right :) Or maybe even entirely right — it's not clear what everything was that you were pointing at.

  17. That's a very good point! I should've made the same remark :) Created an issue: #2827084: EntityNormalizer::denormalize should not throw UnexpectedValueException, but \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException, so we get a 422 response instead of 400. And added it as a @todo.
  18. Heh…
  19. :) Clear, isn't it? :)
  20. :D Fixed, here and everywhere else.
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @Wim Leers for your quick response and astonishing test coverage in this issue. I'll RTBC especially because test coverage is a process, not a state. You should continue working on tests, they aren't finished.

Yes! This was very tricky to figure out, but using getOwner() meant we were calling \Drupal\user\Entity\User::getAnonymousUser(), which generates an in-memory User entity. Which has no UUID set, which meant that the code further down that sets the expected _links normalization would fail, because it expects ['uuid' => ['value' => NULL]], which is of course wrong. See \Drupal\comment\Entity\Comment::getOwner(). Documented this now.

Wow, good to know!

Yes, it's needed. explode(' ', '') === [''], sadly.

oh PHP!

If that's the case, then \Drupal\serialization\Normalizer\EntityNormalizer::denormalize() is also wrong, and we'd need to fix that. (In a separate issue of course, since this one is test-only.) Note how it does:
$entity_type_definition = $this->entityManager->getDefinition($entity_type_id, FALSE);

if ($entity_type_definition->hasKey('bundle')) {

$bundle_type_id = $entity_type_definition->getBundleEntityType();
$bundle_types = $bundle_type_id ? $this->entityManager->getStorage($bundle_type_id)->getQuery()->execute() : [];

// Make sure a bundle has been provided.
if (!is_string($data[$bundle_key])) {
throw new UnexpectedValueException('A string must be provided as a bundle value.');
}

// Make sure the submitted bundle is a valid bundle for the entity type.
if ($bundle_types && !in_array($data[$bundle_key], $bundle_types)) {
throw new UnexpectedValueException(sprintf('"%s" is not a valid bundle type for denormalization.', $data[$bundle_key]));
}
(i.e. any entity type definition that has a bundle key can have that bundle type unexpected error response occur, as long as there is a bundle entity type)
… oh but wait, it's still possible to get the A string must be provided as a bundle value error response even when there is no bundle entity type! So you're partially right :) Or maybe even entirely right — it's not clear what everything was that you were pointing at.

Well bundles are seeded by hook_entity_bundle_info(), entity types is just one of the possible storages for those bundles.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 121: rest_comprehensive_tests-2737719-121.patch, failed testing.

Wim Leers’s picture

Mixologic’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 121: rest_comprehensive_tests-2737719-121.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Random fail in OutsideInBlockFormTest: https://www.drupal.org/pift-ci-job/535605

catch’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
14.91 KB

I found some minor code style/comment nits but lost them before I could post anything, and nothing important.

I had some questions which were all answered in the issue summary, so just recording here my internal conversation:

1.
Q. Why isn't this removing any tests?
A. Because there's a follow-up for that to keep the patch under 250kb at #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase. I asked one question on there about whether we can additionally remove some HAL tests, but given this is a significant refactoring, that's fair enough to do it in two issues. However it'd be good to get the removal in quickly to avoid duplicate test coverage.

2.
Q. Why isn't it using data providers so we can have fewer test classes?
A. One to make it easy for contrib/new core modules to re-use the tests. Also not everything could be done with a data-provider anyway.

It feels like there still might be a way to handle #2, but a lot of sub-classes is still a lot better than lots of one-off tests that are incomplete anyway, and I can see Wim already tried this at one point in the issue.

So while the scope of the patch is large, since this adds test coverage for pending bug reports (albeit commented out) and allows further testing improvements down the line, I think it's reasonable to get it in.

However, phpcs isn't happy at all, and it's too much to do on commit, so cnw for that.

Wim Leers’s picture

Fixed all phpcs violations!

I asked one question on there about whether we can additionally remove some HAL tests, but given this is a significant refactoring, that's fair enough to do it in two issues. However it'd be good to get the removal in quickly to avoid duplicate test coverage.

+1

So while the scope of the patch is large, since this adds test coverage for pending bug reports (albeit commented out) and allows further testing improvements down the line, I think it's reasonable to get it in.

:) :) :) This will help so much with making API-first Drupal a reality rather than an aspiration!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 129: rest_comprehensive_tests-2737719-129.patch, failed testing.

Wim Leers’s picture

I asked one question on there about whether we can additionally remove some HAL tests, but given this is a significant refactoring, that's fair enough to do it in two issues. However it'd be good to get the removal in quickly to avoid duplicate test coverage.

Already did this: #2824576-7: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Gah, again that random fail in OutsideInBlockFormTest. Issue for that: #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly. Retesting.

Wim Leers’s picture

Sigh. #2789315: Create EntityPublishedInterface and use for Node and Comment landed despite breaking BC. Testing #129 against 8.3. It should fail now.

Wim Leers’s picture

#2789315: Create EntityPublishedInterface and use for Node and Comment was reverted. That removed several failures.

But, testing #129 made it clear that 8.3.x already deviated from 8.2.x in several ways. So, rolling a separate 8.3.x patch…

  1. #2821013: EntityResource's field validation 403 message is inconsistent between POST and PATCH apparently was already fixed by #2291055: REST resources for anonymous users: register in 8.3.x.
  2. Drupal core in 8.3.x vs 8.2.x apparently has different header lowercasing handling. Content-language became content-language, X-Drupal-Cache became x-drupal-cache etc. Opened #2831388: Minor regression: ResourceResponseSubscriber lowercases all header names for that.
  3. The status field of Comment entities is validated before other fields in 8.3.x, due to #2810381: Add generic status field to ContentEntityBase. (This does not constitute a BC break.)
  4. On 8.2.x, HAL+JSON Comment responses have the url.site cache context, but not in 8.3.x. The root cause is \Drupal\Core\EventSubscriber\RedirectResponseSubscriber running after \Drupal\Core\EventSubscriber\FinishResponseSubscriber: the url.site cache context is on the response object, but it's there too late. This is a gap in the test coverage of #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty, which landed against 8.3… except that that patch does have test coverage: ResourceResponseSubscriberTest. The problem is that there's no proper functional test coverage that tests the integration; the interaction with other parts of the system. And that's exactly what this patch provides! So, I'm including the fix here (since I can't actually add it in a separate issue; that'd require me duplicating big portions of the test coverage being introduced here, which defeats the purpose of this issue/patch providing comprehensive test coverage).

So, attached:

  1. rest_comprehensive_tests-2737719-129--8.2.x.patch, this is identical to the patch in #129, but renamed to clarify that this must be committed to 8.2.x only.
  2. interdiff-8.3.x.txt: contains the changes to compensate for the changes in 8.3.x versus 8.2.x
  3. rest_comprehensive_tests-2737719-134--8.3.x.patch, this is identical to the patch in #129, plus the 8.3.x-specific interdiff.
Wim Leers’s picture

The interdiff explained:

  1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php
    @@ -42,9 +42,9 @@
    +    'status',
         'created',
         'changed',
    -    'status',
         'thread',
         'entity_type',
         'field_name',
    
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
    @@ -28,6 +28,7 @@
        * {@inheritdoc}
        */
       protected static $patchProtectedFieldNames = [
    +    'status',
         'pid',
         'entity_id',
         'uid',
         'homepage',
         'created',
         'changed',
    -    'status',
         'thread',
         'entity_type',
         'field_name',
    

    This is for <a href="#comment-11801731">#134</a>.3.

  2. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -196,7 +196,8 @@ protected function flattenResponse(ResourceResponseInterface $response) {
    -    $events[KernelEvents::RESPONSE][] = ['onResponse'];
    +    // Run shortly before \Drupal\Core\EventSubscriber\FinishResponseSubscriber.
    +    $events[KernelEvents::RESPONSE][] = ['onResponse', 5];
    

    This is for #134.4.

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -384,7 +384,7 @@ public function testGet() {
    -    $ignored_headers = ['Date', 'Content-Length', 'X-Drupal-Cache', 'X-Drupal-Dynamic-Cache'];
    +    $ignored_headers = ['Date', 'Content-Length', 'X-Drupal-Cache', 'x-drupal-dynamic-cache'];
    

    This is for #134.2.

  4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -589,8 +589,7 @@ public function testPost() {
    -    // @todo Add trailing period in https://www.drupal.org/node/2821013.
    -    $this->assertResourceErrorResponse(403, "Access denied on creating field 'field_rest_test'", $response);
    +    $this->assertResourceErrorResponse(403, "Access denied on creating field 'field_rest_test'.", $response);
    

    This is for #134.1.

  • catch committed 59781ff on 8.3.x
    Issue #2737719 by Wim Leers, dawehner: EntityResource: Provide...
Wim Leers’s picture

I dug deeper into #134.3, i.e. I finished the investigation proposed by #2831388: Minor regression: ResourceResponseSubscriber lowercases all header names. Turns out this is also caused by #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty. And just like it makes sense to fix #134.4 (that other regression caused by #2831388), it makes sense for this too. No need for excessive explicit test coverage… because that's exactly what the purpose of this very issue is.

The only mistake that we made is committing #2831388 before this patch.

We really, truly, desperately need this patch to land.


Redoing what I did in #134 with this new knowledge. You must manually compare the two interdiff-8.3.x.txt files. But they're tiny, so that's okay. That way, the interdiff here continues to show the difference between the 8.2.x and the 8.3.x patch.

  • catch committed c1667f3 on 8.2.x
    Issue #2737719 by Wim Leers, dawehner: EntityResource: Provide...

The last submitted patch, 137: rest_comprehensive_tests-2737719-129--8.2.x.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 137: rest_comprehensive_tests-2737719-136--8.3.x.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Fixed

#137 was cross-posted with @catch already committing this, even before the patch came back green. He got distracted/confused.

13:55:16 <WimLeers> catch: So!
13:55:19 <WimLeers> catch: I propose this
13:55:19 <catch> WimLeers: so I was just going to say the tests shouldn't depend on the case at all, and that's the bug, and we shouldn't worry about what case we send.
13:55:38 <WimLeers> catch: 1. wait 10 minutes, 2. it should come back green, 3. move my last reroll for 8.3.x to a follow-up
13:55:44 <catch> WimLeers: +1

I can see that it already came back green, so we're good :)

So, moving #137 to a follow-up. Reopening #2831388: Minor regression: ResourceResponseSubscriber lowercases all header names for that.

(Naturally, this explains the fails for #137's patches: they don't apply anymore.)

The last submitted patch, 134: rest_comprehensive_tests-2737719-134--8.3.x.patch, failed testing.

Wim Leers’s picture

Status: Fixed » Needs work

#137's 8.3.x patch failed. The commits need to be reverted.

EDIT: even though it's all green locally… this may be hard to figure out. I'm currently suspecting a random bot fail.

  • catch committed 3c4e905 on 8.3.x
    Revert "Issue #2737719 by Wim Leers, dawehner: EntityResource: Provide...
Wim Leers’s picture

Retesting all patches in #134 and #137

Wim Leers’s picture

Fails reproduced. I can only reproduce this fail when I specify the BROWSERTEST_OUTPUT_FILE environment variable while running the test. It fails to determine the caller by introspecting the backtrace. Investigating that.

The last submitted patch, 137: rest_comprehensive_tests-2737719-136--8.3.x.patch, failed testing.

The last submitted patch, 134: rest_comprehensive_tests-2737719-134--8.3.x.patch, failed testing.

Wim Leers’s picture

Title: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method » [PP-1] EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
Status: Needs work » Postponed

Root cause found. #2763401: PHPunit browser tests should log all Mink requests was committed to 8.3.x only (so far, there's a patch for 8.2.x too that's not yet committed), and had one major flaw, which is the one that's causing exactly the failures that we see in the failures against 8.3.x here:

Undefined offset: 1

/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:1850
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:439
/var/www/html/vendor/guzzlehttp/promises/src/FulfilledPromise.php:39
/var/www/html/vendor/guzzlehttp/promises/src/TaskQueue.php:61
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:246
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:223
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:266
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:225
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:62
/var/www/html/vendor/guzzlehttp/guzzle/src/Client.php:129
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:298
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:292

The solution is #2822387: Undefined offset 1 in BrowserTestBase::getMethodCaller(). So this patch is now blocked on #2822387.

Wim Leers’s picture

Title: [PP-1] EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method » EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
Status: Postponed » Reviewed & tested by the community

#2822387: Undefined offset 1 in BrowserTestBase::getMethodCaller() landed. This is now unblocked. Retesting the last two patches.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 137: rest_comprehensive_tests-2737719-136--8.3.x.patch, failed testing.

The last submitted patch, 134: rest_comprehensive_tests-2737719-134--8.3.x.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Requeueing again, because #2822387: Undefined offset 1 in BrowserTestBase::getMethodCaller()'s commit had not yet been pushed. Canceled the retests (hence the two fails), now restarted them. Fingers crossed!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 137: rest_comprehensive_tests-2737719-136--8.3.x.patch, failed testing.

The last submitted patch, 134: rest_comprehensive_tests-2737719-134--8.3.x.patch, failed testing.

Wim Leers’s picture

Well, from 106 fails to 34. That's better. But not the zero we were expecting.


Most fails are caused by the infuriating insanity that is \Psr\Http\Message\StreamInterface::getContents(). It doesn't return the contents, like the method says. It returns the remaining contents.

So apparently something in HEAD has changed since yesterday, that causes the response body to already have been read.

Changing all occurrences (there are five) of $response->getBody()->getContents() to (string) $response->getBody() to fix that. I could change it only for the 8.3.x patch, but it's a unnecessary divergence, unlike all the other things in interdiff-8.3.x.txt. So, updating the 8.2.x patch too.

The attached interdiff applies to both patches. The interdiff-8.3.x.txt interdiff at #137 still shows the difference between the 8.2.x and 8.3.x patches.

Status: Needs review » Needs work

The last submitted patch, 156: rest_comprehensive_tests-2737719-156--8.3.x.patch, failed testing.

Wim Leers’s picture

I found the root cause for the failing PageCacheTest.

The PageCache middleware expects the Expires header to be set, always (which seems like a bug, but an out of scope one for sure). It is normally set by FinishResponseSubscriber::onRespond() because setResponseCacheable() is called because isCacheControlCustomized() returns FALSE.

The changes made to ResourceResponseSubscriber in interdiff-8.3.x.txt cause that to change. Well, they don't cause it, they expose it. The problem is with HEAD's \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::flattenResponse(), specifically

$final_response->headers->add($response->headers->all());

Specifically, ResponseHeaderBag::add() calls ResponseHeaderBag::set() which calls ResponseHeaderBag::add() which calls ResponseHeaderBag::computeCacheControlValue(). That code does this:

        // public if s-maxage is defined, private otherwise
        if (!isset($this->cacheControl['s-maxage'])) {
            return $header.', private';
        }

Because it decided unilaterally to add the , private suffix (despite it being told not to!), this causes FinishResponseSubscriber::isCacheControlCustomized() to conclude that cache control was customized:

  protected function isCacheControlCustomized(Response $response) {
    $cache_control = $response->headers->get('Cache-Control');
    return $cache_control != 'no-cache' && $cache_control != 'private, must-revalidate';
  }

Man, what a nightmare. Recap:

  1. a freshly instantiated Symfony response returns Cache-Control: no-cache (despite not specifying that no-cache value)
  2. if you then duplicate that response using all the proper Symfony APIs, you'll get Cache-Control: no-cache, private, simply because we passed in the no-cache value we got from the original response

This makes no sense at all.

This explains why Symfony itself doesn't use add() + all(), they use clone. Switching to cloneing the headers fixes the problem.


In this reroll, which should finally be green on 8.3.x:

  1. the 8.2.x patch is unchanged compared to #156, so reuploading the same one
  2. the interdiff-8.3.x.txt interdiff is what changes, specifically the line related to headers. This line was already being modified (since #137), as of now it's being modified differently.
  3. the 8.3.x patch is changed compared to #156: that one line was modified.

  • catch committed 3190767 on 8.3.x
    Issue #2737719 by Wim Leers, dawehner: EntityResource: Provide...

  • catch committed 4b1327d on 8.2.x
    Issue #2737719 by Wim Leers, dawehner: EntityResource: Provide...
catch’s picture

Status: Reviewed & tested by the community » Fixed

OK the interdiffs are quite minor even if the issues they expose aren't. Committed/pushed to 8.3.x and 8.2.x, nice to finally get this in (again).

Wim Leers’s picture

FileSize
208.76 KB

vaplas’s picture

FileSize
91.26 KB

Stunning performance!

ultra llama mode on

dawehner’s picture

Congrats @Wim Leers!

almaudoh’s picture

@WimLeers +10^10000! Nicely done :)

klausi’s picture

Wim Leers’s picture

@vaplas: OMG! :D Thanks so much, I've added that to my permanent collection of llama pictures! Where did you find this? Did you create it yourself?
@dawehner: Thanks! And thanks for your help here in the form of reviews!
@almaudoh: :) Thanks!
@klausi: I'm on it.

e0ipso’s picture

@Wim Leers thanks for the unmeasurable amount of work dedicated to this! We are so much stronger after this.

I hope you enjoyed your Graham's!

Wim Leers’s picture

Thanks, and yes I did :)

vaplas’s picture

#168, only slightly corrected the existing Llama.

And a couple of Friday's analogies for the llamas of the Drupal core:

Postponed:

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

webchick’s picture

Issue tags: +8.3.0 release notes