Skip to content

PB-946 part 2: backfill test for stack notification batcher#776

Merged
zhming0 merged 1 commit into
mainfrom
ming/pb-946-part-2
Dec 1, 2025
Merged

PB-946 part 2: backfill test for stack notification batcher#776
zhming0 merged 1 commit into
mainfrom
ming/pb-946-part-2

Conversation

@zhming0
Copy link
Copy Markdown
Contributor

@zhming0 zhming0 commented Nov 26, 2025

Back fill test coverage for notification batcher.

@zhming0 zhming0 requested a review from a team November 26, 2025 06:13
@zhming0 zhming0 requested a review from a team as a code owner November 26, 2025 06:13
Comment thread api/fake_agent_server.go Outdated
}

// Wait for ticker to flush (interval is 100ms)
time.Sleep(250 * time.Millisecond)
Copy link
Copy Markdown
Contributor

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 ?

Copy link
Copy Markdown
Contributor Author

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.Sleep or synctest.Wait.

Sadly "durably blocked" does not include:

System calls like file or network I/O
External event handling (such as reading from a socket)

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.

Comment thread api/stack_notification_batcher_test.go Outdated
@zhming0 zhming0 requested a review from DrJosh9000 November 27, 2025 04:06
Comment thread api/stack_notification_batcher_test.go Outdated
@zhming0 zhming0 force-pushed the ming/pb-946-part-2 branch from 6e5486a to 7a787ca Compare December 1, 2025 03:32
@zhming0 zhming0 enabled auto-merge December 1, 2025 03:32
@zhming0 zhming0 merged commit eab654d into main Dec 1, 2025
1 check passed
@zhming0 zhming0 deleted the ming/pb-946-part-2 branch December 1, 2025 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants