Testing: use synctest for timing-dependent tests#756
Testing: use synctest for timing-dependent tests#756La002 wants to merge 8 commits intomodelcontextprotocol:mainfrom
Conversation
Converted tests with time.After/time.Sleep to use Go 1.25's synctest package in new *_go125_test.go files for instant, deterministic execution. Tests using real I/O remain timing-based. Fixes modelcontextprotocol#680
La002
changed the title
|
@maciej-kisiel Pushed with the changes discussed here |
maciej-kisiel
left a comment
maciej-kisiel
left a comment
There was a problem hiding this comment.
Thank you for the PR and and patience with the review. I added some comments.
mcp/cmd_go125_test.go
Outdated
|
|
||
| // wait for the server to exit | ||
| // synctest will detect deadlock if server doesn't exit | ||
| synctest.Wait() |
There was a problem hiding this comment.
Is synctest.Wait() required here? If I understand synctest's documentation correctly, I believe blocking on onServerExit should be sufficient.
There was a problem hiding this comment.
Yes, I did misunderstand the behaviour of synctest.Wait(). Thanks for all the review comments
Removed all the places it wasn't needed bb74dc2
mcp/elicitation_go125_test.go
Outdated
| }) | ||
|
|
||
| cs, ss, cleanup := basicClientServerConnection(t, c, nil, nil) | ||
| _ = cs // Dummy usage to avoid "declared and not used" error. |
There was a problem hiding this comment.
I know it is pre-existing, but maybe we can leave the campsite better:
Wouldn't putting _ in place of cs one line above work as well?
mcp/elicitation_go125_test.go
Outdated
| } | ||
|
|
||
| // 3. Client should receive the notification | ||
| synctest.Wait() |
There was a problem hiding this comment.
Similarly here, we're blocking on a channel, so I don't think synctest.Wait() call is required.
mcp/mcp_go125_test.go
Outdated
| waitForNotification := func(t *testing.T, name string) { | ||
| t.Helper() | ||
| time.Sleep(notificationDelay * 2) | ||
| synctest.Wait() |
There was a problem hiding this comment.
Similarly here, I believe it should work without synctest.Wait().
|
|
||
| // ===== resources ===== | ||
| t.Log("Testing resources") | ||
| if runtime.GOOS != "windows" { |
There was a problem hiding this comment.
Can we leave the TODO as a comment?
| "github.com/google/jsonschema-go/jsonschema" | ||
| ) | ||
|
|
||
| func TestEndToEnd_Synctest(t *testing.T) { |
There was a problem hiding this comment.
Could you leave the following TODO for me above the function signature?
// TODO: split this test into multiple test cases that target specific functionality
It was already big before your modifications and will be even harder to debug now that we don't have t.Run calls that would separate the results.
mcp/mcp_go125_test.go
Outdated
| errs <- err | ||
| }() | ||
| time.Sleep(2 * time.Millisecond) | ||
| if i < batchSize-1 { |
There was a problem hiding this comment.
I'm confused, if batchSize is 1, would this if ever execute? It seems the only possible value for i is 0, so this will be 0 < 1-1 = 0 which is false. If you can confirm this is never executed, let's just remove this test.
There was a problem hiding this comment.
Yes its not executed. The comment above also mentions the buggy situation
Removed as part of 246ae8b
| defer cs.Close() | ||
|
|
||
| // Let the connection establish properly first | ||
| time.Sleep(30 * time.Millisecond) |
There was a problem hiding this comment.
Could this be replaced with synctest.Wait()?
| // Wait for keepalive to detect the failure and close the client | ||
| // Check periodically with simulated time advancement | ||
| for i := 0; i < 40; i++ { // 40 iterations * 25ms = 1 second total | ||
| time.Sleep(25 * time.Millisecond) |
There was a problem hiding this comment.
I don't have the background why this loop was introduced, but wouldn't a single sleep longer than the keepalive time be sufficient?
There was a problem hiding this comment.
I think it might have been useful for earlier test, to exit early and not have to wait the whole duration of whatever longer sleep time would have been specified. But now with synctest, time is not an issue, so a sufficient sleep time of 1s should be good enough
mcp/mcp_go125_test.go
Outdated
| cancel() | ||
|
|
||
| // Wait for all goroutines to be blocked (cancellation delivered). | ||
| synctest.Wait() |
There was a problem hiding this comment.
I believe it should work without synctest.Wait().
|
replied to comments @maciej-kisiel |
3 participants
Testing: use synctest for timing-dependent tests
Converted timing-based tests to use Go 1.25's synctest for instant, deterministic execution with simulated time.
Moved converted tests to *_go125_test.go files. Tests using real I/O (exec.Command, httptest) remain timing-based as they require actual system operations.
Fixes #680