fix: harden integration timing and SIGTERM assertions#2847
Merged
markphelps merged 1 commit intomainfrom Mar 18, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates integration tests to be less flaky and more precise about timing/health behavior during async operations and shutdown scenarios.
Changes:
- Replace fixed sleep with repeated health-check polling in the webhook delivery failure test.
- Tighten
predict_timeassertions for async generator pre-collection to require ~0.5s+ duration. - Add stricter SIGTERM exit verification in the concurrent integration test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| integration-tests/tests/webhook_delivery_failure.txtar | Polls /health-check repeatedly during webhook retry/failure window instead of sleeping. |
| integration-tests/tests/async_generator_precollect.txtar | Updates predict_time regex to enforce minimum expected generation duration. |
| integration-tests/concurrent/concurrent_test.go | Adds SIGTERM exit-type/signal assertions using errors.As + syscall.WaitStatus. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+505
to
+520
| if err == nil { | ||
| t.Fatal("server exited cleanly after SIGTERM; expected termination by signal") | ||
| } | ||
|
|
||
| var exitErr *exec.ExitError | ||
| if !errors.As(err, &exitErr) { | ||
| t.Fatalf("server exited with unexpected error type after SIGTERM: %T (%v)", err, err) | ||
| } | ||
|
|
||
| ws, ok := exitErr.Sys().(syscall.WaitStatus) | ||
| if !ok { | ||
| t.Fatalf("server exited after SIGTERM but wait status was unavailable: %v", err) | ||
| } | ||
| if !ws.Signaled() || ws.Signal() != syscall.SIGTERM { | ||
| t.Fatalf("server exit = %v, want signal %v", ws, syscall.SIGTERM) | ||
| } |
michaeldwan
approved these changes
Mar 18, 2026
Member
michaeldwan
left a comment
There was a problem hiding this comment.
looks good, though we should use testify assert/require helpers instead of t.Fatal etc
| case err := <-done: | ||
| t.Logf("Server exited: %v", err) | ||
| if err == nil { | ||
| t.Fatal("server exited cleanly after SIGTERM; expected termination by signal") |
Member
There was a problem hiding this comment.
testify assert/require can cut down a lot of this 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.
Summary
webhook_delivery_failurewith health-check polling to reduce timing flakeasync_generator_precollectto assertpredict_timereflects full generation durationTestSIGTERMDuringSetupto assert termination is specifically caused bySIGTERMTest Plan
go test -tags integration ./integration-tests/concurrent -run TestSIGTERMDuringSetup -count=1python/cog.egg-infoandcrates/target/release/.cargo-lockpermission denied)