[v3] feat: reuse a connected player across test boundaries - #52
Merged
Conversation
npmPackageNameOf assumed the last @ was always the version separator. git+ssh://git@host/repo and https://user:pass@host/pkg carry @ of their own, so two different URL specs got truncated to the same wrong key and collided in the map. Reported by Drownek on PR Drownek#50.
cmd.exe strips the outer pair of quotes from the whole command line when the line starts with a quote and holds more than two quotes total. quoteForCmd now adds quotes to protect ^ in version ranges, so a spaced npm.cmd path (already quoted by Java) plus one quoted argument hits that case and cmd mangles the path. call makes the line start with a letter instead of a quote, which cmd doesn't touch. Reported by Drownek on PR Drownek#50.
Adds an opt-in mechanism to hand a test a bot that survived from an
earlier test instead of reconnecting for every one. Matching goes by
ability labels (op, gamemode:*, and anything a plugin marks by hand)
rather than resetting server state, since the runner has no way to
undo what a command changed.
- lib/player.ts: abilities set, mark()/unmark(); makeOp/deOp/
setGameMode label automatically. makeOp also now recognizes an
already-op player on a stdio/full console, which reuse can hand it
a second time.
- lib/player-registry.ts: PlayerRegistry — resolves a request against
free entries, evicts LRU at maxPlayers, transparently rejoins a
dead connection before handing it out, invalidates on failure.
- lib/session.ts: owns the registry; disconnectAllBots takes a keep
list so a per-test sweep leaves other live registry entries alone.
- lib/test-registry.ts, lib/types.ts, lib/plugin.ts: reuse option on
test()/describe(), TestContext.invalidatePlayer, TestResult.reuse,
PlugwrightPlugin.onPlayerReuse, PluginTestRef.reuse.
- lib/test-runner.ts, runner.ts: resolves the primary player and any
createPlayer() call through the registry when reuse applies;
computes the default maxPlayers from account pool capacity; reports
a per-test reuse summary line.
- lib/config.ts: tests.reuse config, PLUGWRIGHT_REUSE env override.
- gradle-plugin: reuse { enabled, maxPlayers } extension block,
threaded through to every environment's tests.reuse.
- auth-authme-package: preflight declares reuse: false — its whole
point is proving the login flow runs, which a reused, already
authenticated player would skip.
- docs: reuse coverage across writing-tests, configuration,
test-filtering, plugins, external-servers, custom-modes, reports.
Verified against example_plugin's local suite both ways
(PLUGWRIGHT_REUSE=1 and unset): 47/47 either way. Three specs needed
reuse: false / excludeAbilities to keep their isolation assumptions
once reuse was on, annotated with why.
Reuse held a bot connected from the test that created it until the run ended,
which is not something a server that kicks an idle player allows. The only way
out was to switch reuse off entirely, and with it the account, the nick and the
ability labels — none of which have anything to do with idling.
`stay` splits the two apart. Left alone it is `true` and nothing changes.
`false` parks the registry entry instead of holding its connection: the bot
leaves at the end of every test, the entry keeps the identity, and the next test
matched to it gets a `rejoin()` first. What carries over is the identity, not
the connection.
It is settable for the run (`tests.reuse.stay`, `reuse { stay }`,
PLUGWRIGHT_REUSE_STAY) and for one test (`reuse: { stay }`). An environment
forces it with `capabilities.playerReuse = 'rejoin'`, a middle value between the
`true` and `false` that field already had, for a server that objects to an idle
bot but not to a reused one. Neither the config nor a test overrides that, the
same way `false` already outranks `tests.reuse.enabled`.
The registry now calls `env.beforeJoin()` before a rejoin, which it never did. A
rejoin skipping an external server's join throttle was easy to miss while it
happened once in a run on a dead connection; under `stay: false` it happens on
every test.
Registers a one-time setup step per reuse pool (the same string used
as reuse: 'pool' / { key: 'pool' }). PlayerRegistry.resolve/createEntry
now take an onFreshEntry callback, fired only when an entry is actually
(re)built - first-ever request for a key, or a rebuild after a drop -
never on a plain checkout of an already-live entry. A rejected
onFreshEntry discards the half-built entry so the next attempt retries
initialization instead of handing out a broken player.
Wires PlayerRegistry's onFreshEntry into test-runner.ts: when a fresh registry entry is being built for a key with a registered reuseTest, run it with its own timeout/finalizers/plugin fixtures, report it as its own test result via onExtraResult (appended ahead of the test that triggered creation), and propagate failure so the dependent test fails too instead of the whole run crashing on an uncaught rejection. Exports reuseTest from the package's public API alongside test/opTest.
reuseTest now takes (pool, fn) or (pool, options, fn) with the same requires/environments TestOptions carries, plus describe nesting for its name and beforeEach/afterEach hooks - same scope as test/opTest, minus reuse itself (a reuseTest initializes a pool, it doesn't resolve into one). requires/environments skip it exactly like a regular test: reported skipped, fn never runs, no error - a player handed out under a pool whose reuseTest doesn't apply on this environment still connects, just never gets initialized. Extracted the environments+requires skip check (previously inline in runner.ts's skipReasonFor) into lib/skip-reason.ts so runFile and reuseTest execution share one implementation instead of two copies drifting apart. runner.ts now threads environmentName through to test-runner.ts so reuseTest's own skip check has something to compare against.
Message buffers are per player, and `rejoin` empties one on the way back in. A player checked out under `stay` never left, so it never rejoins and never gets that clear — its chat from the previous test stayed in the buffer and could satisfy an assertion in the next one. Nothing caught it before because the buffer was session-wide and every test started by clearing it. Per-player buffers are the right shape, but they moved that clear out from under reuse, so reuse now does it where it belongs: beside the closed window, in core's safe minimum for a returning player.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fifth in the series (#46), on top of #50.
Two fixes that didn't make it into #50
Reported by you (Drownek) during #50's review, fixed and pushed to that PR's branch —
but the fixes landed after #50 was already merged, so they never made it into
v3-dev. Re-applied here as the first two commits: the npm package-name dedupwrongly cut a git/URL spec at its own
@(colliding two different packagesinto one key), and Windows'
cmd /cwas stripping the outer quotes off analready-quoted
npm.cmdpath oncequoteForCmdstarted wrapping versionranges.
Player reuse
Opt-in mechanism (
reuse: true, and areuse { enabled, maxPlayers }gradleextension block) to hand a test a bot that survived from an earlier test
instead of reconnecting for every one. Matching goes by ability labels (op,
gamemode:*, plugin-marked), not by resetting server state — the runner has no
way to undo what a command changed.
PlayerRegistryresolves against freeentries, evicts LRU at
maxPlayers, transparently rejoins a dead connectionbefore handing it out.
stay(defaulttrue) controls whether the bot holds its connection betweentests or leaves the server and rejoins on the next match — for servers that
kick an idle player. An environment can force
capabilities.playerReuse = 'rejoin'.Verified against example_plugin's suite both ways (
PLUGWRIGHT_REUSE=1andunset): 47/47 either way.
reuseTestA pool gets its own one-time setup step, run only when its registry entry is
actually built (first request, or a rebuild after a drop) — never on a plain
checkout of a live entry.
reuseTesttakes the sameTestOptionsscope astest/opTest(requires/environments, describe nesting, before/afterEach)minus reuse itself, and is reported as its own test result, a dependency of
the test that triggered it.
One follow-up fix once this was exercised in the example suite: a trigger
test's chat-history buffer wasn't cleared on checkout when the player never
actually left the server under
stay.Docs
writing-tests.mdxgets full reuse + reuseTest coverage; reuse also touchedconfiguration, test-filtering, plugins, external-servers, custom-modes and
reports docs for the option's cross-cutting bits.
Merging
Same request as the rest of the series: merge commit or rebase and
merge, not squash and merge — later PRs in the series stack on this one.