fix: repair broken test blocking all PR CI#67
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a CI-blocking regression in Copilot engine tests by aligning the test with the engine’s actual lifecycle (where Start() is invoked via Initialize(), not Execute()).
Changes:
- Updates the failing timeout-related test to call
Initialize()and assert start error wrapping/propagation. - Adjusts test setup/mocking to reflect the corrected call path (and adds an import for constructing the mocked error).
Contributor
|
@richardpark-msft I've opened a new pull request, #68, to work on those changes. Once the pull request is ready, I'll request review from you. |
The test was mocking Start() expecting Execute() to call it, but Execute() calls CreateSession() directly — Start() is only called by Initialize(). Fixed to test Initialize() error propagation directly, which is the actual regression guard for the deadlock fix. This test failure was blocking CI on all open PRs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename TestCopilotExecute_StartRespectsTimeout to TestCopilotExecute_InitializePropagatesStartError to match what the test actually asserts - Use errors.New instead of fmt.Errorf for constant string (staticcheck S1028) - Remove unused fmt import Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dd13355 to
a899545
Compare
richardpark-msft
approved these changes
Mar 4, 2026
spboyer
added a commit
to spboyer/waza-fk
that referenced
this pull request
Mar 5, 2026
* fix: repair broken TestCopilotExecute_StartRespectsTimeout mock setup The test was mocking Start() expecting Execute() to call it, but Execute() calls CreateSession() directly — Start() is only called by Initialize(). Fixed to test Initialize() error propagation directly, which is the actual regression guard for the deadlock fix. This test failure was blocking CI on all open PRs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address review feedback — rename test, use errors.New - Rename TestCopilotExecute_StartRespectsTimeout to TestCopilotExecute_InitializePropagatesStartError to match what the test actually asserts - Use errors.New instead of fmt.Errorf for constant string (staticcheck S1028) - Remove unused fmt import Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
chlowell
pushed a commit
to chlowell/waza
that referenced
this pull request
Mar 5, 2026
Also, shuffled some gitignore rules around so we're not ignoring our own source code.
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.
Problem
TestCopilotExecute_StartRespectsTimeoutfails on every PR — the mock expectsStart()to be called duringExecute(), butExecute()callsCreateSession()directly.Start()is only called byInitialize().This was introduced by PR #43's merge and is blocking CI on all 7 open PRs.
Fix
Changed the test to exercise
Initialize()(which actually callsStart()) and verify error propagation. The regression guard for the deadlock fix (#31) is preserved — just targeting the correct code path now.Verified
go test ./... -count=1— all tests pass