docs(run-once): fix the examples that violate the key rule - #149
Conversation
… silently
runOnce keys its flag file by the string alone, in a directory shared by every
project in the run:
const flagDir = path.join(os.tmpdir(), `playwright-once-${process.ppid}`);
const flagFile = path.join(flagDir, `${key}.done`);
Nothing in the key comes from the project. So when one spec is matched by more
than one project -- which is what adding an `-app-next` lane does -- the first
project's setup satisfies the second, and the second skips configure() and
deploy() entirely. It then fails much later on a missing element, with nothing
pointing at the cause. That happened on rhdh-plugin-export-overlays#3318 and
was caught in review; four workspaces still pass literal keys today.
Automatic per-project scoping would be wrong, because both intents are real:
installing an operator into a fixed namespace that every project then uses
genuinely wants once per run, while anything touching a project's own namespace
wants once per project. The API cannot guess, so this makes the caller say:
await test.runOnce("my-setup", fn, { scope: "project" });
Default behaviour is unchanged.
For the keys that stay run-scoped, the dangerous case is no longer silent. The
flag file now records which project satisfied it, and a skip on behalf of a
*different* project logs a warning naming both and pointing at the option --
because that is nearly always the mistake rather than the intent.
Tests are new; there were none for this helper. Each was checked by mutation:
collapsing the scoped key back to `key` fails the per-project test, dropping the
warning guard fails the warning test, and warning unconditionally fails the
same-project test. Full suite 117/117, lint clean.
Part of RHIDP-16456.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Version Bump Check requires both for any change under src/, and it caught that I had pushed neither. Documents the new scope option and the silent skip it fixes. Part of RHIDP-16456. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI runs `yarn check`, which is typecheck + lint:check + prettier:check. I had run lint and the tests separately and missed the formatting gate, so the new test file failed it. `yarn check` is the command to run locally. Only the test file changed: prettier:check excludes docs/, so the changelog needed no reformatting, and running --write over it had touched two unrelated entries. Part of RHIDP-16456. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e applied
The guides still said a runOnce key must be unique across "all spec files
and projects", and their two flagship examples wrapped configure() and
deploy() with no scope — which is exactly the shape that leaves a second
project with no deployment. Someone reading the guide rather than the
changelog would have written the bug.
Both examples now pass { scope: "project" }, the key section explains what
scope decides instead of asserting the old rule, and a new Scope section
gives the run-vs-project table plus the reason deploy() needs no scope of
its own: its internal key already carries the namespace.
Also close the one silent path the option still had. With scope "project"
and no resolvable project the key fell back to the bare form and behaved as
run-scoped again, without a word. It now warns, and the unit test for it
fails when the warning is removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…project scope cannot be honoured
Review of the first two commits found the warning firing on the one case the
docs call correct, and the scope option still able to degrade quietly.
The warning fired on every cross-project skip of a run-scoped key, including
"install an operator into a fixed namespace every project then uses" — the
example the docs give for choosing "run". In an N-project run that printed N-1
warnings telling the author to switch to { scope: "project" }, which for that
setup would mean N operator installs, and there was no way to opt out because
the guard tested the resolved scope. It now tests `options.scope` as passed:
undefined means the author never thought about it, which is who the advice is
for; an explicit "run" is an answer and is left alone.
{ scope: "project" } with no resolvable project used to warn and then fall back
to the bare key — which is the shared-key bug it exists to prevent, reported by
one line easy to lose in CI output. It throws now. That only reaches callers
outside a Playwright context, since test.info() resolves inside beforeAll.
The flag file records which project satisfied a key, and the lock-free fast
path reads it from another process. writeFileSync truncates before it writes,
so a reader landing between the two steps saw an empty file and dropped the
warning — in the two-projects-one-spec case, the exact case it exists for.
Written through a temp file and renamed.
Docs: the earlier pass missed four files, including the whole overlay section,
which is the one rhdh-plugin-export-overlays follows and where the bug was
reported. Every documented runOnce block that wraps deploy() now passes the
scope — audited rather than spot-fixed — and the API reference documents the
third argument at all, which it did not. Also noted that nesting does not
rescue a missing scope: a run-scoped outer call skips before deploy() is
reached, so its internal protection never gets a say.
The changelog now leads with the fact that upgrading alone changes nothing:
the default is unchanged by design, so each affected call site has to opt in.
Both new behaviours were mutation-tested — reverting each turns exactly one
test red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Two review rounds landed on this branch since it opened. Summary for whoever picks it up. What changed in the behaviour
The warning no longer fires on a deliberate The flag file is written atomically. Its content is what names the project in the warning, and the lock-free fast path reads it from another process. DocumentationThe first pass missed four files, including the entire Also recorded that nesting does not rescue a missing scope — a run-scoped outer call skips before VerificationBoth new behaviours are mutation-tested — reverting each turns exactly one test red:
And end to end, one spec under two projects: Note the third column: an explicit One thing worth stating plainlyUpgrading to 2.1.10 does not fix an affected call site. The default is unchanged by design, so every existing Also: |
|
Hey, I’m not sure we need this in the helper. In 2.1, the default one is the new front-end system, then why we are still running the old test cases in that way. If we have migrated successfully, then we can just remove the older one and use the new one, right? Overlay CI still runs both shells on the same spec (plugin and plugin-app-next), which is the collision we already hit (rhdh-plugin-export-overlays#3318). If we drop the legacy project once app-next is the default, that skip goes away. Until then two projects in one Playwright run still share one runOnce flag dir. Could we just put namespace in the key like deploy() already does (deploy-${namespace})? Something like runOnce isn’t tied to Playwright today, which is why deploy() and the unit tests can use it. { scope: "project" } plus test.info() couples it to a test, and defaulting to "run" means existing overlay keys stay broken until every caller opts in (a warning doesn’t actually run setup). What if we keep runOnce generic, update the examples, and let call sites do what deploy() already does? its easy and no worries of regression. |
Review is right and this reverts the code change entirely.
The rule this guide already stated was correct — "the key must be globally
unique across all spec files and projects... use a prefix that includes the
workspace or project name". What was wrong is that every example underneath it
then used a literal key around configure() and deploy(), violating the rule the
paragraph had just given. Someone reading the examples rather than the sentence
wrote the bug; the earlier version of this branch responded by changing the
sentence, which was the wrong half to move.
So runOnce goes back to what it was. The { scope: "project" } option, the
import of @playwright/test into a module that had no test-runner dependency,
and the version bump are all gone. deploy() and the unit tests can keep using
runOnce outside a Playwright context, which was the point of it being generic.
What lands is the documentation:
- the key section now says why a project-shared key breaks, quoting the flag
directory that is keyed on the runner PID alone, and shows the fix as
`${key}-${rhdh.deploymentConfig.namespace}` — which is what deploy() has
always done internally, and why deploy() was never affected;
- both intents are spelled out side by side, because both are real: a namespaced
key for setup that belongs to one project, a literal one for an operator
installed into a fixed namespace every project then uses;
- every example that wraps deploy() carries the namespace — twelve of them,
across the guide, the API reference, the deployment guide and all three
overlay pages;
- nesting is noted as no rescue: a project-shared outer key skips before
deploy() is reached, so its internal protection never gets a say.
Not carried over: the warning when a shared key is skipped for another project.
It needed test.info(), which is the coupling being reverted. That gap is better
closed in rhdh-plugin-export-overlays, where a static check can find a literal
key in a workspace whose config declares two projects over one spec — before a
deployment rather than after one.
RHIDP-16456.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#149 was reduced to a documentation change, so it no longer bumps the version and 2.1.10 is free. The note about the two PRs colliding goes with it — there is only one version-bumping PR open now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gustavolira
changed the title
#151 merged today and claimed 2.1.10, so the renumber in the previous commit collided with main and the Version Bump Check would have refused it. Same number as before, different reason: this is no longer about #149, which now bumps nothing at all. Worth noting separately: main says 2.1.10 but the publish workflow for it failed, so npm latest is still 2.1.9. If that is not resolved before this merges, 2.1.11 publishes over a 2.1.10 that never existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You're right — reverted the whole code change. I had it backwards. The rule in the guide was already correct; what was broken is that every example under it used a literal key around Small thing, not to reopen it: "existing keys stay broken until callers opt in" is equally true of adding What does get lost is the warning that found the bad call sites, since it needed One back at you: dropping the legacy lanes once app-next is default is RHIDP-16460, still New and unassigned. Until someone owns it the two-project window stays open. |
|
/lgtm |
2 participants
Documentation only after review — the API change is reverted.
git diff origin/main -- src/ package.jsonis empty.The guide already had the right rule:
Every example under it then used a literal key around
configure()+deploy(), breaking the rule the paragraph had just given. That is what was worth fixing.What changed
`${key}-${rhdh.deploymentConfig.namespace}`, which is whatdeploy()does internally and whydeploy()was never affected.deploy()is reached.Not here: the warning that found affected call sites needed
test.info(), which is the coupling being reverted. That moved to redhat-developer/rhdh-plugin-export-overlays#3375 as a static check.yarn checkclean, 111 tests, docs build clean.