-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
don't use external service for testing websockets #1138
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. You mentioned something about moving the httpmultibin
to its own package - do you want to do it in this PR, or in a separate PR, and then merge the resulting master
into #1007 (for which that's important)?
Hmmm .. I technically did directly in #1117 but I can cherry-pick it in a different PR and rebase it when this get merged ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
3f54c7f
to
e108a97
Compare
After a bunch more tries at fixing the panics and getting ... them rarer and rarer I am of the opinion that it's a lot better if I just remove the logs and assert/require's in the ws handlers. I don't think that the handlers themselves should check for this things I think that the test should be able to detect whether the handle did what they wanted and if this is not possible a custom handler and that is synchronized to exist before the test ends will need to be written. |
Codecov Report
@@ Coverage Diff @@
## master #1138 +/- ##
==========================================
- Coverage 73.58% 73.55% -0.03%
==========================================
Files 144 144
Lines 10527 10527
==========================================
- Hits 7746 7743 -3
- Misses 2323 2326 +3
Partials 458 458
Continue to review full report at Codecov.
|
3e4daa7
to
2c27c56
Compare
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.
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.
part of #537