Problem

  1. Drupal\config\Tests\ConfigImportAllTest takes 5.4 minutes (9%) of the total core test suite time.

  2. But yet, it does not cover any concrete test cases.

  3. That is not acceptable in any way.

Proposed solution

  1. Remove the test without replacement. It is useless.

  2. If any regressions MAY appear in the future, add discrete test coverage for those cases.

Details

  1. This test is useless, because it is a "wildcard" test — instead of testing discrete functionality/regressions, it doesn't know what to test, so it blatantly tests each and everything in the hope that it will catch "something".
  2. I've studied the test code to see what expectations are actually covered. (see below)

Known test coverage

  1. None that wouldn't be covered by existing configuration system tests already.
CommentFileSizeAuthor
test.configall.0.patch4.26 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#1808248-99: Add a separate module install/uninstall step to the config import process tried to address the problem, which has been raised in the original issue already:

this explicitly tests that all non-required modules can be enabled through the config importer and that the default configuration created by a standard install can also by created reliably through the importer.

If you do not want to rely on standard profile - what we could do is enable all the modules and then import the configuration from the standard profile. But in my mind this is still relying on the standard profile.

Further more I'd like to add the converse test that all non-required modules and their configuration and config entities can be removed by the config importer.

(paragraphs added by me)

All of those test cases are covered by more targeted tests already. Standard profile has no special meaning and must not have a special meaning.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Related issues: +#2234159: Fix config importing through the UI and change ConfigImportAll test using the UI

#2234159: Fix config importing through the UI and change ConfigImportAll test using the UI slightly adjusts this test to become even slower (→ WebTestBase).

At the same time, it adds a discrete test case to ConfigImportUITest to assert that the config.module itself cannot be uninstalled in a config import (when config is imported through the UI).

→ Again, concrete expectations and regression tests are covered in more targeted tests. ConfigImportAllTest is useless.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

You're wrong - it is covering importing a set complete site in the best way possible. Without this test we would not have caught #2232275: System menu blocks need to be able to depend on their menu entities. And continues to do so - the issue with batching and config import remains to be solved see #2234159: Fix config importing through the UI and change ConfigImportAll test using the UI

Be my guest to add a test that test all the interactions between all the config entities created by enabling all the modules and their interaction on import. Then we can remove this test.

xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Major » Normal

Yeah, I consider this issue a wontfix, for the reasons described in #3.

Also this is not a major in any case.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Normal » Major

A single test that consumes 10% of the total core test suite time without any concrete benefit presents a major issue in my book.

#2232275: System menu blocks need to be able to depend on their menu entities wasn't mentioned at all in #1808248: Add a separate module install/uninstall step to the config import process, the change was only merged/incorporated at some point.

And again, #2232275 added discrete test coverage for a discrete test case. That's proper test coverage.

If you insist on keeping this test, then pretty please, can I get an answer to these two simple questions:

1. What exactly is being tested by ConfigImportAllTest that is not covered by other tests already?

2. In case there is anything, why is that not covered by explicit tests?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

What exactly is being tested by ConfigImportAllTest that is not covered by other tests already?

What is being covered is the general interaction of enabling every module through the config importer - there is no other test that enables all enableable modules through the importer. I would argue that this test should become even longer since I would like to add disabling all the modules too.

Importantly this is also testing the interaction when we try to do this all in one go - pretending the modules do not couple themselves together in interesting and hard to predict ways doesn't make sense to me. Enabling modules and creating configuration especially config entities are how drupal sites couple themselves.

In case there is anything, why is that not covered by explicit tests?

As this test finds / catches new stuff we should be adding explicit tests - just as we did for #2232275: System menu blocks need to be able to depend on their menu entities. Additionally it should be testing using the UI as we can prove that HEAD would fail this at the moment #2234159: Fix config importing through the UI and change ConfigImportAll test using the UI. Since HEAD is not failing we obviously don't have a testcase for this.

Furthermore testbots run with a concurrency of 8 so quoting a percentage and a time is slightly disingenuous since removing this test will not speed up the tests by 5.4 minutes.

ParisLiakos’s picture

ParisLiakos commented

If your patch fails on this test and you try to run this locally..well...good luck. It was really frustrating for me last night, trying to realize why my patch failed...and in the end it was alexpott who figured it out.

  • It tries to enable all modules, even the ones that are under /modules directory. So before running this test you need to disable those modules and move them out of your drupal root.
  • It always reaches the max_execution_time.. 240s is not enough..the test timed out after running for several minutes.(my laptop is an i5 toshiba with 12gb ram..so its not a matter of old/crappy hardware).
  • I gave up
  • If alexpott hadn't figure out why my patch failed this test, i would give up the issue i was working on, too

We cant ping alexpott everytime we have a problem with this test...at least, lets have it test a hardcoded set of modules with interdependencies, than every possible module that is in the filesystem.

xjm’s picture

xjm
she/her
Primary language English
commented

@ParisLiakos, it sounds like the test caught a bug in that patch, and was the only one that did, so it seems to me like it served its purpose there.

Maybe we should add documentation to the test explaining how to debug with it, though in some ways it doesn't seem any different than any web test where I would try to reproduce the bug by following the test's steps manually in an actual Drupal installation.

ParisLiakos’s picture

ParisLiakos commented

@xjm I wouldnt call the fact that you have two field items with same weight (0) a bug. At least Field UI does not enforce anything like that..you can easily go to manage display tab of your entity set two items weight to 0, save, export it..Nothing will ever complain.
And generally in Drupal when we specify two items with weight 0 means we dont particularly care for sorting.

So if this was a bug:
a) Field UI should not allow it
b) Exporting fields like that should not be possible
c) Have a dedicated PHPUnit test for this, thats parses all yml configurations in config/install files and checks for duplicate weights.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Closed (won't fix)

This test has demonstrated its usefulness time and again by catching otherwise-undiscoverable bugs very quickly, and the cost of having it is not that high. And why would we remove test coverage that has caught a bug even once? I'm going to mark this wontfix. If you still disagree, we can escalate the issue, but I really don't think it is worth further argument.