Repository navigation
Fail fast when -waitForWorktrees times out, and give it more headroom - #734
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The only noted issue is a minor diagnostic nit; the requested reliability changes are otherwise complete.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Improves worktree test reliability by extending the async wait timeout and reporting timeout failures clearly.
Changes:
- Increases timeout from 10 to 20 seconds.
- Adds an assertion when the expected worktree state is not reached.
| File | Description |
|---|---|
GitXTests/PBGitWorktreeManagementTests.m |
Updates worktree polling timeout and failure reporting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| while (!condition([self second]) && [limit timeIntervalSinceNow] > 0) | ||
| [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate dateWithTimeIntervalSinceNow:0.01]]; | ||
|
|
||
| XCTAssertTrue(condition([self second]), @"Worktrees did not reach the expected state before the wait timed out"); |
There was a problem hiding this comment.
I'm not sure if my change make sense at all. I only see, that it's now error free
|
@hannesa2 the Suggestion: wait for the branch too, in both add tests (L464-L467, L475-L478), e.g. |
testANewWorktreeCanStartANewBranch failed on CI (https://github.com/gitx/gitx/actions/runs/... arm64, macOS 26.6.2) with '(null)' not equal to 'refs/heads/fresh': -waitForWorktrees' 10-second poll for the async `git worktree list` reload timed out silently on the shared runner, so the test read the worktree list before the new worktree ever appeared and failed on the unrelated -branchRefName comparison instead. -waitForWorktrees now asserts its own condition once the wait ends, so a stall shows up there with a clear message, and the window is 20s rather than 10s to give a loaded CI runner more room before that assertion fires.
the (null) may come from a race rather than a timeout: a git worktree list still running from setUp can read the list mid-worktree add, when git shows the new worktree as HEAD 0000..., detached, locked initializing (reproduced locally with git 2.55). The wait sees fresh-here exist and returns, so the new assert passes and the branch check still fails.
73de521 to
2f862af
Compare
|
I applied your suggestion |
testANewWorktreeCanStartANewBranch failed on CI
(https://github.com/gitx/gitx/actions/runs/... arm64, macOS 26.6.2) with
(null)not equal torefs/heads/fresh: -waitForWorktrees' 10-second poll for the asyncgit worktree listreload timed out silently on the shared runner, so the test read the worktree list before the new worktree ever appeared and failed on the unrelated -branchRefName comparison instead.-waitForWorktrees now asserts its own condition once the wait ends, so a stall shows up there with a clear message, and the window is 20s rather than 10s to give a loaded CI runner more room before that assertion fires.