Testing: use synctest for timing-dependent tests

[La002]

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

[La002]

@findleyr @jba following up, is this still relevant ? Are any changes required?

[La002]

@maciej-kisiel Pushed with the changes discussed here

[maciej-kisiel]

Thank you for the PR and and patience with the review. I added some comments.

[maciej-kisiel]

Could this be replaced with synctest.Wait()?

[La002]

Added 246ae8b

[maciej-kisiel]

Does the test still pass if you remove the Sleep? If so, I would propose to remove it altogether (the comment may be left to explain the synctest.Wait call).

[maciej-kisiel]

I don't have the background why this loop was introduced, but wouldn't a single sleep longer than the keepalive time be sufficient?

[La002]

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

[maciej-kisiel]

Let me try to be more precise: could this loop be replaced with:

time.Sleep(51 * time.Millisecond) // One millisecond more than the keepalive duration.
if _, err := cs.CallTool(...); !errors.IsError(err, ErrConnectionClosed) {
  t.Errorf(...)
}

[La002]

I think it should not be ideally. When the connection is closed it should need some buffer time to detect the failure. (2-3x atleast should have been given)
But I am unable to test that here because of this potential issue, when we do CallTool() - its also capable of detecting error , whether KeepAlive mechanism detected it or not.
I tried this below test for example (without synctest), and this still passes.

func TestKeepAliveFailure(t *testing.T) {
	// same csetup.....
	defer cs.Close()

	// Let the connection establish properly first
	//time.Sleep(30 * time.Millisecond)

	// simulate ping failure
	ss.Close()
	
	time.Sleep(10 * time.Nanosecond) // net wait from session closure is 10 ns
	_, err = cs.CallTool(ctx, &CallToolParams{
		Name:      "greet",
		Arguments: map[string]any{"Name": "user"},
	})
	if errors.Is(err, ErrConnectionClosed) {
		return // Test passed
	}
	t.Errorf("expected connection to be closed by keepalive, but it wasn't. Last error: %v", err)
}

So a bigger wait of 51ms is also passing.
Is this expected?

[La002]

replied to comments @maciej-kisiel

[maciej-kisiel]

Thank you for the changes. There are still 3 unresolved comment threads, please take a look.

[maciej-kisiel]

Does the test still pass if you remove the Sleep? If so, I would propose to remove it altogether (the comment may be left to explain the synctest.Wait call).

[maciej-kisiel]

Let me try to be more precise: could this loop be replaced with:

time.Sleep(51 * time.Millisecond) // One millisecond more than the keepalive duration.
if _, err := cs.CallTool(...); !errors.IsError(err, ErrConnectionClosed) {
  t.Errorf(...)
}