Problem/Motivation

There are 143 content entity REST tests in core, and each one tests GET, POST, PATCH and DELETE as individual methods. Each of these methods requires a separate Drupal install in the test.

If we combine these four test methods into one, we save 3 installs per test. On my machine this cuts a single test class down from 40 seconds to 24 seconds. In total this should cut out 429 installs in a full test run.

Steps to reproduce

Run e.g. UserJsonCookieTest

Proposed resolution

Combine the test methods.
Clean up after each method so the next method can run without a full reinstall.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3575888

Command icon Show commands

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

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

About issue forks

Comments

longwave created an issue. See original summary.

longwave opened merge request !14922

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
Related issues: +#3572055: Fix skip for unsupported methods on REST tests

#3572055: Fix skip for unsupported methods on REST tests also will help speed up these tests.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This looks really encouraging. I think I tried something similar at some point but balked at the need to tear down after each protected method, but what's here is pretty minimalist vs. the time saving. Can't see anything to complain about so RTBC for me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

As we're changing test base classes I checked out contrib and couldn't find anywhere that this change was going to cause failures but I'd be very surprised if this change doesn't break a contrib or custom test somewhere. This feels an impossible change to make in a 100% BC manor - other than creating completely new test base classes to extend which I don't think is warranted here given what I've seen in contrib. Perhaps a change recrod with suffice.

catch’s picture

catch
he/him
Primary language English
commented
Title: Combine HTTP method tests in EntityResourceTestBase » Combine HTTP method tests in A
Status: Needs work » Reviewed & tested by the community

Added a CR https://www.drupal.org/node/3578858

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Combine HTTP method tests in A » Combine HTTP method tests in EntityResourceTestBase
godotislate’s picture

godotislate
he/him
commented

One minor question on the MR.

godotislate closed merge request !14922

  • godotislate committed a4b46f33 on main 
    refactor: #3575888 Combine HTTP method tests in EntityResourceTestBase...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed a4b46f3 and pushed to main. Thanks!

Did not apply to 11.x, so need a backport MR.

  • alexpott committed 93bc0d96 on 11.x 
    task: #3575888 Combine HTTP method tests in EntityResourceTestBase

By:...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Patch (to be ported) » Fixed

I was merging at the same time. The conflicts were simple to resolve so I did.

Committed 93bc0d9 and pushed to 11.x. Thanks!

I resolved the 11.x conflict during merge.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed df3ae27b on 11.x 
    Revert "task: #3575888 Combine HTTP method tests in...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Fixed » Needs work

Well that was overly confident. @godotislate was correct to ask for a backport MR.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

Pesky tests we'd removed due to the contact module being removed...

alexpott opened merge request !15091

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

11.x appears to be passing.

  • catch committed 2edc50e8 on 11.x 
    task: #3575888 Combine HTTP method tests in EntityResourceTestBase

By:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !15091

Status: Fixed » Closed (fixed)

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