-
Notifications
You must be signed in to change notification settings - Fork 51
PB-946 part 2: backfill test for stack notification batcher #776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| package api | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "log/slog" | ||
| "net/url" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/buildkite/stacksapi" | ||
| "github.com/google/go-cmp/cmp" | ||
| "github.com/google/go-cmp/cmp/cmpopts" | ||
| ) | ||
|
|
||
| func TestNotificationBatcher_SendsBatchesPeriodically(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| server := NewFakeAgentServer() | ||
| defer server.Close() | ||
|
|
||
| serverURL, _ := url.Parse(server.URL()) | ||
| client, err := stacksapi.NewClient("fake-token", stacksapi.WithBaseURL(serverURL)) | ||
| if err != nil { | ||
| t.Fatalf("NewClient() error = %v", err) | ||
| } | ||
|
|
||
| nb := newNotificationBatcher("test-stack", client, slog.Default()) | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| if err := nb.start(ctx); err != nil { | ||
| t.Fatalf("start() error = %v", err) | ||
| } | ||
|
|
||
| // Add notifications | ||
| want := []stacksapi.StackNotification{ | ||
| {JobUUID: "job-A", Detail: "detail A"}, | ||
| {JobUUID: "job-B", Detail: "detail B"}, | ||
| {JobUUID: "job-C", Detail: "detail C"}, | ||
| } | ||
| for _, n := range want { | ||
| if err := nb.add(ctx, n); err != nil { | ||
| t.Fatalf("add() error = %v", err) | ||
| } | ||
| } | ||
|
|
||
| // Wait for ticker to flush (interval is 100ms) | ||
| time.Sleep(250 * time.Millisecond) | ||
|
|
||
| if got, wantLen := len(server.NotificationCalls), 1; got != wantLen { | ||
| t.Fatalf("number of notification calls = %d, want %d", got, wantLen) | ||
| } | ||
|
|
||
| if diff := cmp.Diff(want, server.NotificationCalls[0], cmpopts.IgnoreFields(stacksapi.StackNotification{}, "Timestamp")); diff != "" { | ||
| t.Errorf("notifications mismatch (-want +got):\n%s", diff) | ||
| } | ||
|
|
||
| cancel() | ||
| nb.waitDone() | ||
| } | ||
|
|
||
| func TestNotificationBatcher_ChunksBatchesBySize(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| server := NewFakeAgentServer() | ||
| defer server.Close() | ||
|
|
||
| serverURL, _ := url.Parse(server.URL()) | ||
| client, err := stacksapi.NewClient("fake-token", stacksapi.WithBaseURL(serverURL)) | ||
| if err != nil { | ||
| t.Fatalf("NewClient() error = %v", err) | ||
| } | ||
|
|
||
| nb := newNotificationBatcher("test-stack", client, slog.Default()) | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| if err := nb.start(ctx); err != nil { | ||
| t.Fatalf("start() error = %v", err) | ||
| } | ||
|
|
||
| // Add more than maxNotificationsPerBatch (1000) notifications | ||
| for i := range 2500 { | ||
| note := stacksapi.StackNotification{ | ||
| JobUUID: fmt.Sprintf("job-%d", i), | ||
| Detail: "detail", | ||
| } | ||
| if err := nb.add(ctx, note); err != nil { | ||
| t.Fatalf("i=%d add(%v) error = %v", i, note, err) | ||
| } | ||
| } | ||
|
|
||
| // Wait for ticker to flush (interval is 100ms) | ||
| time.Sleep(250 * time.Millisecond) | ||
|
|
||
| // Should be chunked into 3 calls: 1000 + 1000 + 500 | ||
| if got, want := len(server.NotificationCalls), 3; got != want { | ||
| t.Fatalf("number of notification calls = %d, want %d", got, want) | ||
| } | ||
|
|
||
| // Each batch should be at most 1000 | ||
| for i, batch := range server.NotificationCalls { | ||
| if len(batch) > 1000 { | ||
| t.Errorf("batch %d size = %d, want <= 1000", i, len(batch)) | ||
| } | ||
| } | ||
|
|
||
| // Total should be 2500 | ||
| total := 0 | ||
| for _, batch := range server.NotificationCalls { | ||
| total += len(batch) | ||
| } | ||
| if got, want := total, 2500; got != want { | ||
| t.Errorf("total notifications = %d, want %d", got, want) | ||
| } | ||
|
|
||
| cancel() | ||
| nb.waitDone() | ||
| } | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a better mechanism for ensuring the notifications have flushed?
Could we take advantage of https://pkg.go.dev/testing/synctest ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL synctest, I gave it a good play. I don't think it works in our use. The basic premise of synctest is like poorman's DST, within
synctest.Test's when all goroutines are "durably blocked", time flies fast, it fast track to a time where at least one goroutine can be blocked, and then fast forward to next etc. As if a single threaded cooperative concurrency model.The test goroutine can also enter durably blocked state by issuing
time.Sleeporsynctest.Wait.Sadly "durably blocked" does not include:
This is precisely what the agent faker server is doing.
As a result, our fake agent server (blocked on the real IO connection accept) and client (blocked on readLoop/writeLoop) will never enter the "durably blocked" state, meaning the entire synctest block will be blocked. (Remember, it's single threaded, one running routine can cause the entire thread to block).
I think it's in theory possible to instrument fake agent server and client so it can become "durably blocked" during test, but I don't think the benefits justify the effort yet.