Skip to content
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

Fix occasional spurious test failure. #418

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Jan 9, 2023

Along the lines of 3fc5065, just with a different test: occasionally, with the race detector and other tests running in parallel, I will get a timeout here. But I think that's just that there's no reason in principle this can't take more than two seconds, so under extreme circumstances it may. So this patch removes the timeout; if there's a real bug the channel receive will just hang and eventually the test-suite-wide timeout will catch it.

Along the lines of 3fc5065, just with a
different test: occasionally, with the race detector and other tests
running in parallel, I will get a timeout here. But I think that's just
that there's no reason in principle this can't take more than two
seconds, so under extreme circumstances it may. So this patch removes
the timeout; if there's a real bug the channel receive will just hang
and eventually the test-suite-wide timeout will catch it.
@zenhack
Copy link
Contributor Author

zenhack commented Jan 9, 2023

...tacked on a second commit fixing a similar problem in another test.

Note that the first of these I have seen in CI a few times.

Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@lthibault
Copy link
Collaborator

lthibault commented Jan 9, 2023

=== RUN   TestPipelineCall
Error:     server_test.go:272: GetNumber() #1 = 1; want 0
Error:     server_test.go:278: GetNumber() #2 = 0; want 1

This is new, isn't it? ☝️

@lthibault lthibault merged commit cb2c5a8 into capnproto:main Jan 9, 2023
@lthibault
Copy link
Collaborator

lthibault commented Jan 9, 2023

I re-ran the tests and they passed, so I went ahead and merged.

@zenhack
Copy link
Contributor Author

zenhack commented Jan 9, 2023

Yeah, that is new -- pretty confident this isn't the source, but we should dig into that.

@zenhack zenhack deleted the spurrious-test-failure branch January 9, 2023 20:23
@lthibault lthibault mentioned this pull request Jan 10, 2023
@lthibault
Copy link
Collaborator

I've opened #420 to track it.

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