fix: scope the Playwright server state to a single run - #245
Open
yeapea wants to merge 1 commit into
Open
Conversation
The server description lives at one path per project, so two test runs in the
same checkout share it. The second run overwrites it, which quietly points the
first run's workers at the second run's browser, and then removes it on
teardown. After that every visit() in the first run fails with
file_get_contents(.../.temp/playwright-server.json): No such file or directory
The file is now scoped to a run id that the main process mints and exports, so
workers resolve their own run and a teardown only removes its own description.
The export needs $_SERVER and $_ENV, not just putenv(). Symfony's Process builds
the inherited environment as array_intersect_key(getenv(), $_SERVER), so a
putenv()-only variable never reaches a ParaTest worker.
Also stops ServerManager::playwright() from calling Port::find() on every call.
Only the first call creates the server, but every call persisted the freshly
found port, so from the second call on the file described the running server by
a port nothing was listening on.
Files left behind by a run that was killed before its teardown are swept after
six hours.
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
1 participant
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.
We hit this on a long browser suite and it took a while to pin down, so here it is with the repro.
AlreadyStartedPlaywrightServerwrites the running server's host and port to one path per project:Every parallel worker reads it on each
visit(). The path has nothing in it that identifies the run, andmarkAsStopped()unlinks it unconditionally. So if two test processes are going in the same checkout, the second one overwrites the description and then deletes it when it finishes. The first run's workers were already talking to the second run's browser at that point, which is why nothing looks wrong until the file disappears and every remaining test dies with:For us that was a suite that ran two catalogue sweeps that overlap. 937 cases, 257 passed, then 680 failed in a row, none of them for a reason that had anything to do with what they were testing.
What changed: the state file now carries a run id that the main process mints and exports, so a worker resolves the server of the run it belongs to, and a teardown can only remove its own description. Files from a run that got killed before its teardown are cleaned up after six hours, otherwise they'd just accumulate.
One detail that cost me an afternoon and might be worth knowing: exporting the run id with
putenv()alone does not work. Symfony'sProcessbuilds the inherited environment asarray_intersect_key(getenv(), $_SERVER), so a variable that only exists ingetenv()gets dropped before a ParaTest worker ever sees it. It needs$_SERVERand$_ENVtoo. The fix looked correct and did nothing until I checked that specifically.I also moved the
Port::find()call inServerManager::playwright()inside the guard. Only the first call creates the server, but every call was finding a fresh port and persisting that one, so from the second call onwards the file described the running server by a port nothing was listening on. In practice the window is small because the second call comes fromterminate(), but it's wrong either way.Three tests in
tests/Unit/Playwright/. They fail on5.xwithout the source change and pass with it, so they're checking the thing and not the wiring.Happy to adjust the approach if you'd rather solve it another way. The run id in an environment variable is the least invasive thing I could find that also works for ParaTest workers, but if there's a mechanism in Pest for this that I missed, I'll gladly use it instead.