Problem/Motivation

EntityResourceTestBase is subclassed by every entity type in core, which is further subclassed nine times for various REST and HAL tests. It contains four test methods, one for each HTTP verb.

However, when testing config entities, three of these HTTP verbs simply pass immediately:

  public function testPost() {
    // @todo Remove this in https://www.drupal.org/node/2300677.
    if ($this->entity instanceof ConfigEntityInterface) {
      $this->assertTrue(TRUE, 'POSTing config entities is not yet supported.');
      return;
    }
...
  public function testPatch() {
    // @todo Remove this in https://www.drupal.org/node/2300677.
    if ($this->entity instanceof ConfigEntityInterface) {
      $this->assertTrue(TRUE, 'PATCHing config entities is not yet supported.');
      return;
    }
...
  public function testDelete() {
    // @todo Remove this in https://www.drupal.org/node/2300677.
    if ($this->entity instanceof ConfigEntityInterface) {
      $this->assertTrue(TRUE, 'DELETEing config entities is not yet supported.');
      return;
    }

The setup of these tests is still done, but no actual testing is performed. This means that for each config entity type we perform 3 x 9 = 27 installs via BrowserTestBase::setUp() that are immediately thrown away because the test is effectively skipped.

At present there are 31 config entity types (and only 13 content entity types) in core. This means that we are performing 31 x 27 = 837 unnecessary installs during every full test run across core.

Steps to reproduce

Proposed resolution

Add a new ConfigEntityResourceTestBase parent class to EntityResourceTestBase for config entities that only require testGet().
Keep EntityResourceTestBase intact for content entities and any downstream users in contrib or custom code.
Modify EntityResourceRestTestCoverageTest to ensure both config and content entities are tested correctly.

Revert this patch when #2300677: JSON:API POST/PATCH support for fully validatable config entities lands.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3212346

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 !632

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review

Also refactored EntityResourceRestTestCoverageTest from a functional to a kernel test.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

This patch appears to shave approx 4 minutes off the functional tests section of each full test run.

Sample run without this patch: https://dispatcher.drupalci.org/job/drupal_patches/83356/consoleFull

14:55:56 Test run duration: 34 min 4 sec

Latest run of this patch: https://dispatcher.drupalci.org/job/drupal_patches/83362/consoleFull

15:45:17 Test run duration: 30 min 2 sec
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: Avoid 837 unnecessary installs when testing config entities » Avoid hundreds of unnecessary installs when testing config entities

I am not sure the 837 is entirely accurate here, because XmlEntityNormalizationQuirksTrait is used 153 times and overrides two test methods to do nothing, which (if my calculations are correct) makes up 306 of the installs, so perhaps only 531 or so are fixed here? Maybe worth tackling those XML ones in a followup.

Spokje made their first commit to this issue’s fork.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community

Since I've only merged the recent commits on 9.3.x-dev into the MR and didn't make any code changes, I _think_ I'm allowed to put this on RTBC?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott’s picture

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

I think this will silently reduce test coverage of anything in contrib or custom that extends from Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase. I'm not sure we can do that. If we want to do this I think we need to make a new class that Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase extends from or deprecate it and build a new class.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Refactored so ConfigEntityResourceTestBase is the base class, used by core config entities, and content entities and contrib/custom config entities extend EntityResourceTestBase as before.

longwave’s picture

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

Update IS with new approach.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Refactored as per @alexpott's suggestion - did not realise we could use getName() in setUp() but that makes things much, much simpler.

alexpott’s picture

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

All the MR applies to 10.x we get...
Fatal error: Declaration of Drupal\Tests\rest\Functional\EntityResource\ConfigEntityResourceTestBase::setUp() must be compatible with Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::setUp(): void in /Volumes/dev/sites/drupal8alt.dev/core/modules/rest/tests/src/Functional/EntityResource/ConfigEntityResourceTestBase.php on line 22

Running one test locally...

Before

Time: 00:14.639, Memory: 10.00 MB

OK (4 tests, 137 assertions)

After

Time: 00:06.277, Memory: 10.00 MB

There were 3 skipped tests:

So this is definitely a win - we just need a 10.0.x patch with setup(): void in the new testbase class and I'll rtbc.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Added void return to the base class, as it's new code and all concrete classes have void return type already I think this is safe.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
alexpott’s picture

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

Thanks @longwave - this looks great.

  • catch committed eacb05b on 10.0.x 
    Issue #3212346 by longwave, Spokje, alexpott: Avoid hundreds of...
catch’s picture

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

Committed/pushed to 10.0.x, thanks!

We can't do a straight backport to 9.4.x with the void return type hint due to PHP 7.3, so I cherry-picked, removed it, rebased with a fixup commit, and pushed that.

Nice find!

  • catch committed b5fbaf7 on 9.4.x 
    Issue #3212346 by longwave, Spokje, alexpott: Avoid hundreds of...
neclimdul’s picture

neclimdul
He/Him
commented
Status: Fixed » Needs work

Not sure how this didn't get caught but this broke tests.

php vendor/bin/phpunit --configuration core --verbose --list-tests

PHP Fatal error:  Access level to Drupal\Tests\datetime_range\Functional\EntityResource\EntityTest\EntityTestDateRangeTest::setUp() must be public (as in class Drupal\Tests\rest\Functional\EntityResource\ConfigEntityResourceTestBase) in /app/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php on line 50

Fatal error: Access level to Drupal\Tests\datetime_range\Functional\EntityResource\EntityTest\EntityTestDateRangeTest::setUp() must be public (as in class Drupal\Tests\rest\Functional\EntityResource\ConfigEntityResourceTestBase) in /app/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestDateRangeTest.php on line 50
neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Fixed

hm... that error is all sorts of weird. trying to figure out _why_ its weird I removed the rewritten classes in my sites/simpletest directory and magically things started working. I'm very confused but it seems to be working so moving back to fixed. Hopefully not a big deal.

Status: Fixed » Closed (fixed)

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