Issue #3618630: Declare a config schema for every plugin that stores settings, and make every wait in the browser tests patient

Implements [#3618630].

Seven plugins kept their settings under a schema key nothing declared. Core keeps a permissive fallback for most of those families, which is what made the gap quiet: field.formatter.settings.* and field.widget.settings.* are a mapping with no keys, so an undeclared formatter or widget saves without complaint and the site then reports every key it actually stored as having no schema. views.filter.* and views.field.* resolve to the base filter and field types, so those two are correct today and would start reporting the moment either plugin defined an option of its own.

A views access plugin has no fallback at all. views.access.yoyaku_order_overview took its display's whole access.options mapping down with it, which leaves a view this project ships invalid out of the box.

The rule is flat: a plugin shipped in one of those families declares its own entry, whether or not it has settings yet. An entry with no keys is the honest answer for a plugin that stores none, it is what core writes for its own (views.access.none), and it means the day a setting is added the schema is already the place it goes. Eight plugins, eight entries, one of which was already there.

The test is the point of the change. It reads the plugin id out of every formatter, widget and views plugin the project ships, works out the schema key each one's settings belong under, and asserts a schema file declares it. Against the code before this commit it fails seven times, once per gap, which is how the list was checked rather than guessed. It walks only the project's own root and its modules/ tree, deliberately: CI builds the Drupal root inside the checkout, so a plain walk would take all of core and every contrib module for one of ours. It reads the id from a named argument, from views' positional form, and from an annotation, so a plugin written either way is covered.

A schema label is a translatable string, which the module's own translation check is what says out loud: the ten new labels carry their French in this commit.

Not in here, and worth knowing when reading a configuration report: a site can also carry keys in its active configuration that no schema declares because the code that wrote them is gone. This project is pre-1.0 and ships no update hooks, so nothing removes them. Those are site data rather than a defect, and the report shows them identically.

Checked: the new test 8/8 with the fix and 7 failures without it. phpcs over the changed files under Drupal,DrupalPractice: clean. phpstan at level 3: clean. cspell: nothing new. The translation check: every string the module ships is translated. And on a site carrying the affected configuration, the three errors it reported for the view and the display are gone after a cache rebuild.


And the reason this MR has a second commit

The pipeline was going red on this branch for a reason that is not this branch's work. The browser tests flake, each red naming a different scenario: the same commit, rerun untouched, passed the one that had failed and failed another. An earlier pipeline failed four across two classes. None is about anything under test; each is a wait that ran out. Nothing here could go green until that was fixed, so it is fixed here.

The cause was a number, written out by hand at every call site. [#3616893] shipped AllowsForALoadedRunner for exactly this, and its docblock says why: a fixed timeout is a bet against the runner rather than against the code, and the CI pod shares two cores with the rest of the suite. It was adopted in a handful of places. Everywhere else a test still wrote 10000, or silently took the ten seconds waitForElement() and assertJsCondition() default to.

Half the trait was dead. It promises that a window the test itself holds open is ADDED to the budget rather than spent out of it, and keeps heldOpenFor to do it with. Nothing ever assigned that property, so patience() was a flat twenty seconds: a scenario that had asked the page to sit on its answer for three seconds waited no longer than one that had not.

So no call site writes a wait any more, which is the point rather than a tidy-up: the number, and the decision about it, exist once. The trait grew the pieces that were missing (patienceMs(), waitForTheElement(), waitForTheVisibleElement(), waitForTheElementToGo(), waitUntilTrue(), assertShows()), holdTheAnswerBack() declares its window, and the reset between scenarios forgets it again so one scenario's window does not slow every later one. Two classes that had no wait helper at all now have one, and the querySelector expressions that were being spelled out per call site are spelled out once.

Left alone deliberately: the five wait() calls that prove a negative, where the number is the premise of the assertion after it rather than a bet on the runner, and the scenario that sets a notice's life to thirty seconds because the life is the thing it tests.

Checked: phpstan at the level 1.x now gates on caught two real defects in this sweep of my own (a converted call left with three arguments where two are taken, twice) before CI ever saw it, which is a fair advertisement for [#3618610]. After those: [OK] No errors, phpcs clean over every changed file, the translation check clean. Locally, the two scenarios that failed today pass, and OfferStepperTest, the class [#3616893] and [#3616956] were both about, passes whole.


The flake, and how it was actually found

Three mechanisms were proposed from reading the source and all three were wrong or incomplete: a stale availability lifetime (tested, changed nothing), a feed-driven month re-render (the opening month comes from page settings, not the feed), and a findAll() snapshot of a grid that gets rebuilt (a real defect, fixed here, but not the cause of the reds). What found it was a sampling harness, which is what should have been built first.

Sampled, the pattern was immediate: when the class fails, EVERY scenario fails, and the first failure is a missing login-form field. resetBeforeScenario() signs in a full-permission visitor; one scenario then called drupalLogin() for a deliberately restricted account, so drupalLogin() logged out first and drove the logout AND login forms in the browser; the next scenario's reset switched back. Two logout-and-login round trips per run through the exact path SignsInOnce was written to avoid, and when one races the run carries on signed in as somebody whose feed is forbidden, so every later scenario draws an empty grid.

The fix is the scenario's own intent, without the account switch: it takes view booking availability away from the visitor's own role, which is the case its docblock describes ("the visitor may reach the page but not the availability route"), and the reset grants it back before EVERY scenario rather than at the end of the one that took it. That containment is the durable half: a scenario that throws can no longer poison the ones after it, which is why one race took a whole class down.

Verified by sampling, not by a badge:

before after
local, two samples of six runs 2 failures, 2 failures 0, 0
CI, one class fanned out over ten pods — 10/10 green

The ten-pod run used a temporary CI patch (parallel: 10, --filter, every other job switched off). It has been dropped: .gitlab-ci.yml on this branch is byte-identical to 1.x.

Edited by Frank Mably

Merge request reports

Loading
Loading