Problem/Motivation

The test job was the longest job in the pipeline by a wide margin, running at eight to nine minutes, and nothing had measured where its time went.

Two things decide that time. The job runs the suite through run-tests.sh --concurrency 8, which forks one worker per test CLASS, so a class is a serial chain nothing can split. And the runner pod is scheduled with a CPU request of 2, so those eight workers share two cores.

The second is what governed. Taking work out of the suite did not move the job: with the browser tests already cut by a third and the longest class halved, the job still finished in the same time as a branch carrying none of it, on a runner the lint jobs showed to be the same speed. Nothing was holding the job up except the cores it was given.

Proposed resolution

Split the job so the suite stops sharing two cores, and stop the tests paying for work nothing reads.

  1. The job runs as two, by test type: the kernel tests, which are about seven tenths of the work, and everything else beside them. Each gets a pod of its own. The types named cover every type run-tests.sh knows, so a test cannot fall between the jobs, and the count of tests across the two has to equal what the one job ran. Two rather than three, because a pod costs about a minute before it runs anything and the functional and unit tests are twenty seconds of work between them.
  2. Kernel tests stop installing entity schemas they never touch. Which ones those are was decided by running each class without them rather than by reading it, and the classes that count queries keep everything they declare, because a test that warms a cache and then asserts that nothing more is read may reach a table on one run and not the next.
  3. Browser tests stop installing a whole site per test. A class's setUp() body is the fixture its tests depend on; the site install is the only granularity the framework offers, not the isolation they use. So the tests of a class become scenarios of one test, and each is handed that fixture again. RunsEveryScenario keeps what merging would otherwise cost: a scenario's failure is recorded rather than thrown, so the ones after it still run and the test fails once naming every scenario that broke.

Eight to nine minutes, to about three.

What is not reduced

929 test bodies before, 929 after. Every body is unchanged: each became a private method called as a scenario, or stayed a test. The assertion count falls, and every part of the fall is checking that a fixture was built rather than checking the module: parent::setUp() asserts once that the install succeeded, drupalLogin() four times that the sign-in worked, and installEntitySchema() once per table that the table it just made exists. What each class loses reconciles exactly against how many installs it no longer pays for.

Isolation is genuinely weaker, and a scenario can no longer be selected on its own with --filter. Those costs, and two more, are set out in the comment below.

Remaining tasks

  • The kernel job is now the longest, so it is where any further gain is. Folding same-shape kernel tests into one table-driven test, running every case and asserting the whole table at once, is the shape that fits: a single comparison still names every case that came out wrong.
  • Nine browser classes still install a site per test. Two of them resisted grouping for reasons not yet understood and keep a site each, which is the right answer while that is true.
  • Splitting the kernel job further gains nothing until the longest browser class is split across files, since that class alone is most of the other job.

AI-Generated: Yes (Claude Code was used to help draft this issue summary and to write the code on the merge request. I reviewed and ran the work myself before posting it.)

Issue fork yoyaku-3616973

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:

Comments

mably created an issue. See original summary.

mably’s picture

Status: Active » Needs review
mably’s picture

What the merge request costs, alongside what it saves. None of this is a reason not to take it, but a reviewer should not have to find it out by reading the diff.

What is not reduced

929 test bodies before, 929 after, across the 145 files touched. Every body is unchanged: each became a private method called as a scenario, or stayed a test. The assertion count falls, and every part of the fall is accounted for by checks that the fixture itself was built, which no longer happen once per test: parent::setUp() asserts once that the install succeeded, drupalLogin() four times that the sign-in worked, and installEntitySchema() once per table that the table it just made exists. Nothing the module does stopped being asserted.

Four things that are weaker

Isolation, by design. Scenarios in a grouped class share one site install and a database that accumulates as they run. Each is handed its class's fixture again through resetBeforeScenario(), but content an earlier scenario created is still there. For the seven classes grouped here that was checked and does not change what they assert; that is evidence rather than proof, and it is the reason nine other classes were left alone.

A single test can no longer be selected. --filter on a scenario name matches nothing, because the scenario is not a test any more. Re-running one failing case means re-running its group. That is a real cost to the usual habit of re-running only what failed, and it is why the seat map picker was grouped into eight tests rather than one: the groups are a dial between how much install time is saved and how little has to be re-run.

A failure can take later scenarios with it. A scenario that fails an assertion is recorded and the rest still run, which is the whole point of the wrapper. A scenario that instead leaves the browser session or the fixture broken can make the ones after it fail as a consequence. They are all reported, so nothing is hidden, but the first failure named is not always the one to fix.

A residual risk on the entity schemas. Which schemas a class does not need was decided by running it without them, and that is weaker evidence than it looks for a test whose code path depends on cache state: it may reach a table on one run and not the next. VenueMapVersionTest did exactly that, passing locally and failing on CI needing yoyaku_slot_tariff. Every class that calls Database::startLog() now keeps all the schemas it declares, all 22 of them, which gives back 93 of the removals. That covers the tests known to have this shape, not a proof that no other class has it. If a Base table or view not found ever appears on a class this touched, that is the cause, and restoring that class's schemas is the fix.

Where sharing an install is sound

Only where a scenario asserts about what it built. Four of the nine classes left alone build something the site can hold only once, so a second scenario met an entity id that already existed, a block placed twice, or a file written beside the one it expected. The fifth kind is quieter and is the one worth remembering: a class whose tests each build their own content and then assert with pageTextContains on a collection page. Grouping those keeps every assertion and still weakens them, because the listing now shows the previous scenario's fixture as well and the text can match the wrong row. That case was caught by the assertion count going up rather than down.

AI-Generated: Yes (Claude Code was used to help draft this comment and to write the code on the merge request. I reviewed and ran the work myself before posting it.)

mably’s picture

Title: Cut the time the test suite takes, starting with what the job actually waits on » Run the test suite as two jobs, and stop the tests paying for installs nothing uses
Issue summary: View changes

  • mably committed 5a092b87 on 1.x
    fix: #3616973 Run the test suite as two jobs, and stop the tests paying...
mably’s picture

Status: Needs review » Fixed

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.

Status: Fixed » Closed (fixed)

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