Run tests with race detection on CI#5792
Open
stefanhaller wants to merge 6 commits into
Open
Conversation
stefanhaller
added
the
maintenance
stefanhaller
changed the base branch from
master
to
synchronize-async-view-rendering
stefanhaller
force-pushed
the
run-tests-with-race-detection-on-ci
branch
3 times, most recently
from
1304871 to
6800790
Compare
stefanhaller
force-pushed
the
synchronize-async-view-rendering
branch
from
45b8511 to
2afc0ec
Compare
stefanhaller
force-pushed
the
run-tests-with-race-detection-on-ci
branch
from
6800790 to
43e1db7
Compare
stefanhaller
force-pushed
the
synchronize-async-view-rendering
branch
from
2afc0ec to
cc923b4
Compare
stefanhaller
force-pushed
the
run-tests-with-race-detection-on-ci
branch
from
43e1db7 to
34f13d6
Compare
When lazygit exits but leaves behind a subprocess that inherited its stderr pipe and has detached from the pty, cmd.Wait() blocks in awaitGoroutines waiting for that pipe to reach EOF -- which never happens while the straggler is alive. With no WaitDelay set, that wait is unbounded, so a single leaked process hangs the whole test binary until the 10-minute global timeout fires and panics. Worse, the timeout discards whatever lazygit wrote to stderr before exiting (a panic, a -race report), which is exactly the output needed to diagnose the failure. This surfaces under -race, where lazygit runs slow enough to widen the window for a spawned command to still be alive when lazygit quits, and it's a blocker for enabling the race detector on CI. Bound the wait with cmd.WaitDelay so Wait force-closes the pipe and returns ErrWaitDelay instead of hanging, surface the captured stderr as the error (falling back to the wait error when nothing was printed), and kill the child's process group on failure so a straggler can't linger into a later test or pile up across a run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The integration test watchdog fails a test if its recording takes longer than 40 seconds. Under the race detector everything runs several times slower, so legitimately slow tests (e.g. a conflicting interactive rebase) blow that budget and fail even though nothing is actually stuck. Key the timeout off a build-tag constant: the `race` tag is set automatically when the binary is built with -race, so a race build gets a 5x-longer budget while a normal build is unchanged, and the two can't drift apart the way a runtime flag would. The base 40s stays in one place; only the multiplier varies by build. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Go's default 10-minute timeout was enough for running integration tests normally (both locally and on CI), but with race detection turned on they can take much longer to run. Increase the timeout unconditionally to 30 minutes; we don't bother making a distinction between race vs. normal, because a longer timeout doesn't hurt (I can't recall having hit the global timeout ever; and we still have the per-test watchdog that kills an individual test after 40s).
Add one extra integration-tests job that runs the whole suite under the race detector. A `race` matrix dimension (default false) plus an include entry adds a single git-latest job with LAZYGIT_RACE_DETECTOR set; races live in lazygit's own Go code rather than in git, so one git version is enough, and using latest skips the git-build steps. The race job skips coverage collection: it's redundant with the non-race latest job and would only slow the -race build down further.
The watchdog only log.Fatal'd with a message, so a hung test told us that it timed out but not where it was stuck -- useless for diagnosing an intermittent deadlock under the race detector. Dump all goroutine stacks to stderr first (the harness surfaces this process's stderr on failure), turning a bare timeout into an actionable stack trace. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
To spot slow or anomalous tests across CI runs, record each test's run duration when LAZYGIT_TEST_TIMING is set (to a file path); run_integration_tests.sh prints them at the end, sorted by slowest first. CI sets it for all integration jobs. The harness appends to a file rather than writing to stdout/stderr because `go test` captures those and only surfaces them with -v, which would drown the signal in every test's verbose logs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
stefanhaller
force-pushed
the
synchronize-async-view-rendering
branch
from
cc923b4 to
0020dbe
Compare
stefanhaller
force-pushed
the
run-tests-with-race-detection-on-ci
branch
from
34f13d6 to
c51cb6b
Compare
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.
Now that #5791 has made integration tests race-free, let's add a CI job that runs them with race detection turned on.