replace real sleeps and a 22.7M-element Python fixture in the detachedSpawn and trellis2 runner suites - #6093
Merged
Merged
Conversation
…6007) detachedSpawn.test.js replaced 13 fixed `setTimeout` sleeps (25-200ms each, paid unconditionally) with a bounded `waitUntil` condition poll on the state each test is actually about: the job is alive under its recorded PID, the cleanup rm landed, the log holds a line. The two `reattachDetached` fixtures that slept a real `sleep 1` inside `sh` now block on a marker file the test writes, so the replay window stays open exactly as long as the test needs it — and the "replayed fires before the live line" assertion is now ordered by the event rather than by winning a race against a one-second sleep. `reapAndCleanDetachedDirs` grew an opts passthrough to `reapDetached` (default unchanged) so the sweep test can use the same 25ms cadence as every other job it spawns instead of the 250ms production default, twice. trellis2GenerateRunner.test.js was not sleeping at all: its fixture built the 22.7M-face decoder mesh as `list(range(...))`, ~300ms of Python list allocation per test and hundreds of MB per worker. Nothing — neither the adapter nor either stub — does more than `len()` on it, so `range` gives the identical numbers for free. Under the parallel-worker contention CI actually runs at, that allocation dominated: the pair measured 28.4s wall before and 3.7s after. Also fixes a vacuous assertion found while bypass-probing the change: the pre-decimation-capture test matched "no pre-decimation mesh was captured", a wording the adapter no longer prints, so it passed with the capture removed entirely. It now matches the real "normal map skipped" message and fails when the capture is disabled.
Owner
Author
|
CI-run confirmation (Server tests job on this PR, Linux runner) — the numbers the issue asked to record: Against the baselines quoted in #6007 — |
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
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.
Summary
Removes the real wall-clock waits from two of the slowest server suites.
server/lib/detachedSpawn.test.js— 13 fixedsetTimeoutsleeps (25–200ms, paid unconditionally) become a boundedwaitUntilcondition poll on the state each test is actually about: the job is alive under its recorded PID, the cleanuprmlanded, the log holds a line. The tworeattachDetachedfixtures that ran a realsleep 1insideshnow block on a marker file the test writes, so the replay window stays open exactly as long as the test needs it. The "replayedfires before the live line" assertion is now ordered by the event instead of by winning a race against a one-second sleep.reapAndCleanDetachedDirsgains an opts passthrough toreapDetached(default unchanged), so the sweep test uses the same 25ms cadence as every other job it spawns rather than the 250ms production default, twice.server/services/imageTo3d/trellis2GenerateRunner.test.js— this one was not sleeping at all. Its fixture built the 22.7M-face decoder mesh aslist(range(...)): ~300ms of Python list allocation per test and hundreds of MB per worker. Nothing (neither the adapter nor either stub) does more thanlen()on it, sorangeyields identical numbers for free. Under the parallel-worker contention CI actually runs at, that allocation dominated everything else.No test is skipped, and no assertion is weakened. One assertion got stronger: bypass-probing the change surfaced that the pre-decimation-capture test matched
no pre-decimation mesh was captured, a wording the adapter no longer prints — it passed with the capture ripped out entirely. It now matches the realnormal map skippedmessage.Test plan
Both suites pass unchanged (45 + 24 tests). Timings from the same machine:
detachedSpawn.test.jsalone, idletrellis2GenerateRunner.test.jsalone, idleThe contended row is the one that matters for CI: the fixture allocation is what turns a 9s pair into a 28s pair once workers compete for memory.
Bypass probes confirming the converted tests still catch their regressions:
cleanuprminfinish()→removes the control dir after the job ends when cleanup is setfails._CAPTURED["source"]intrellis2GenerateRunner.py→captures the pre-decimation mesh for the normal bakefails (it passed before the assertion fix).Also ran
server/services/mediaJobQueue(the only production caller ofreapAndCleanDetachedDirs): 116 tests pass.Closes #6007