fix(cli): remove unnecessary PTY from TestServerCreateAdminUser/Validates#25444
Conversation
The Validates subtest only checks that Run() returns a validation error. It never reads PTY output. The PTY copy goroutine would block on io.Copy reading from the master fd, then race with pty.Close() during t.Cleanup, causing a 10s timeout flake under the race detector.
There was a problem hiding this comment.
Pull request overview
Removes an unused PTY setup from the Validates subtest of TestServerCreateAdminUser to fix a flake under the race detector. The subtest only asserts on the validation error returned by Run() and never reads PTY output; the lingering copy goroutine could race with pty.Close() in t.Cleanup, causing a 10s timeout.
Changes:
- Remove
ptytest.New(t)and the assignments toroot.Stdout/root.Stderrin theValidatessubtest.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
How was the root cause verified? I'm a bit skeptical that the reason is as written given there isn't a debugger analysis involved. The reason I raise this is because LLMs like creating plausible reasons, and if we include that in PR/commit messages, it may poison future LLM use.
While this change seems benign, aren't we just treating a symptom? I imagine we would still see this same class of flake elsewhere if we are to believe the reason/conclusion.
PS. If we don't want the output, we should use os.Discard for stdout and stderr to protect os.Stdout and os.Stderr from any unexpected or future output (also helps make it clear what we're testing).
I spent a few more brain tokens investigating and I think the actual correct fix is to pass a context into the outputExpecter as it creates its own testutil.Context. I'm going to merge this anyway. |
johnstcn
deleted the
cian/plat-224-flake-testservercreateadminuservalidates
branch
Fixes https://linear.app/codercom/issue/PLAT-224
The Validates subtest only checks that
Run()returns a validation error and never reads PTY output. We don't need it in this test, so removing.