Fix(?): flaky TestSentReceivedMetrics test #1149
Conversation
| } | ||
| if noReuseReceived < reuseReceived { | ||
| t.Errorf("noReuseReceived=%f is greater than reuseReceived=%f", noReuseReceived, reuseReceived) | ||
| t.Errorf("reuseReceived=%f is greater than noReuseReceived=%f", reuseReceived, noReuseReceived) |
There was a problem hiding this comment.
This is now correct, right? 😄
The error message noReuseReceived=28325.000000 is greater than reuseReceived=28329.000000 seemed wrong to me :)
Codecov Report
@@ Coverage Diff @@
## master #1149 +/- ##
==========================================
- Coverage 73.61% 73.59% -0.02%
==========================================
Files 144 144
Lines 10527 10529 +2
==========================================
Hits 7749 7749
- Misses 2321 2322 +1
- Partials 457 458 +1
Continue to review full report at Codecov.
|
Why not |
Go ahead, I was just testing with |
This _should_ fix flaky test `TestSentReceivedMetrics`. See https://circleci.com/gh/loadimpact/k6/6474, https://circleci.com/gh/loadimpact/k6/6484 . This issue was introduced in a63bb58 (PR #1138), where the previous implementation of `getWebsocketEchoHandler()` closed the connection after writing, so this change brings that back. It seems that closing the connection creates additional metrics which `TestSentReceivedMetrics` takes into account _sometimes_, hence the flakiness. From discussions with @na--, we agreed to remove the persistent connection implementation of a63bb58 (the `for` loop here) since that matches the previous version, but we'll create a new WS test that tests multiple message passing, since none of our current tests seem to do this.
895a5cb to
ae564fd
Compare
There was a problem hiding this comment.
Post-factum: I am pretty sure there was a test that failed which looked like it is sending multiple messages but looking at all the tests while there are some that seem like they do it .. they don't :(
They just loop multiple times without sending anything or if they send anything they expect an error so probably something else was wrong and I fixed it along the way ...
| @@ -667,7 +667,7 @@ func TestRunTags(t *testing.T) { | |||
| }) | |||
|
|
|||
| group("websockets", function() { | |||
| var response = ws.connect("wss://HTTPSBIN_IP:HTTPSBIN_PORT/ws-echo", params, function (socket) { | |||
| var response = ws.connect("WSBIN_URL/ws-echo", params, function (socket) { | |||
There was a problem hiding this comment.
This should've been WSSBIN_URL although ... apperantly it doesn't matter :(
This should fix flaky test
TestSentReceivedMetrics.See https://circleci.com/gh/loadimpact/k6/6474, https://circleci.com/gh/loadimpact/k6/6484 .
This issue was introduced in a63bb58 (PR #1138), where the previous implementation of
getWebsocketEchoHandler()closed the connection after writing, so this change brings that back.It seems that closing the connection creates additional metrics which
TestSentReceivedMetricstakes into account sometimes, hence the flakiness.Please run
go test -count=1 ./coreon this branch a few times on your local machine to ensure it's fixed. :)There are a few unrelated things to the fix here, but check out the individual commits, and let me know if you prefer another PR for them. I just added them here since they're kind of related.