test: Increase timeout to mitigate non-deterministic test failure#3580
test: Increase timeout to mitigate non-deterministic test failure#3580
Conversation
listener: - before this caused the readWriteTimeout to kick in (rarely) while Accept - as a side-effect: remove obsolete time.Sleep: in both listener cases the Accept will only return after successfully accepting and the timeout that is supposed to be tested here will be triggered because there is a read without a write - see if we actually run into a timeout error (the whole purpose of this test AFAIU) Signed-off-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>
Codecov Report
@@ Coverage Diff @@
## develop #3580 +/- ##
===========================================
- Coverage 64.33% 64.15% -0.18%
===========================================
Files 213 213
Lines 17410 17345 -65
===========================================
- Hits 11201 11128 -73
+ Misses 5292 5291 -1
- Partials 917 926 +9
|
privval/socket_listeners_test.go
Outdated
|
|
||
| func TestListenerTimeoutReadWrite(t *testing.T) { | ||
| for _, tc := range listenerTestCases(t, time.Second, time.Millisecond) { | ||
| var ( |
There was a problem hiding this comment.
| var ( | |
| const ( |
| } | ||
|
|
||
| if have, want := opErr.Timeout(), true; have != want { | ||
| t.Errorf("for %s listener, got unexpected error: have %v, want %v", tc.description, have, want) |
There was a problem hiding this comment.
| t.Errorf("for %s listener, got unexpected error: have %v, want %v", tc.description, have, want) | |
| t.Errorf("for %s listener, got unexpected error: have %v, want timeout error", tc.description, opErr) |
| t.Errorf("for %s listener, have %v, want %v", tc.description, have, want) | ||
| } | ||
|
|
||
| if have, want := opErr.Timeout(), true; have != want { |
There was a problem hiding this comment.
| if have, want := opErr.Timeout(), true; have != want { | |
| if !opErr.Timeout() { |
There was a problem hiding this comment.
I like to have this consistent with the cases above (would you also change to if opErr.Op != "read")
There was a problem hiding this comment.
The reason for this consistent form is that when you reformulate the expectations, you can't forget to change the error reprted. Less error prone and less editing.
There was a problem hiding this comment.
for %s listener, got unexpected error: have false want true
in it's current form does not make sense for person running tests
There was a problem hiding this comment.
@xla agree if you're talking about above case where we compare Op == read, but not here
There was a problem hiding this comment.
Agreed, it doesn't always produce the most readable output. A balance must be struck.
Signed-off-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>
xla
left a comment
There was a problem hiding this comment.
Confusing how an accept could take longer than that, but assuming a noisy environment full of little docker whales will be slower than what 50 years of silicon are capable of. The only thing I'd be vary of is that we mask structural issues with the code by just bumping the timeout, if we are sensitive towards that it warrants invesigation, but again this might only be true in the environment our CI runs in.
👍
…endermint#3580) This should fix tendermint#3576 (ran it many times locally but only time will tell). The test actually only checked for the opcode of the error. From the name of the test we actually want to test if we see a timeout after a pre-defined time. ## Commits: * increase readWrite timeout as it is also used in the `Accept` of the tcp listener: - before this caused the readWriteTimeout to kick in (rarely) while Accept - as a side-effect: remove obsolete time.Sleep: in both listener cases the Accept will only return after successfully accepting and the timeout that is supposed to be tested here will be triggered because there is a read without a write - see if we actually run into a timeout error (the whole purpose of this test AFAIU) Signed-off-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> * makee local test-vars `const` Signed-off-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> ## Additional comments: @xla: Confusing how an accept could take longer than that, but assuming a noisy environment full of little docker whales will be slower than what 50 years of silicon are capable of. The only thing I'd be vary of is that we mask structural issues with the code by just bumping the timeout, if we are sensitive towards that it warrants invesigation, but again this might only be true in the environment our CI runs in.
This should fix #3576 (ran it many times locally but only time will tell). The test actually only checked for the opcode of the error. From the name of the test we actually want to test if we see a timeout after a pre-defined time.