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(ws): wrongly sending nil as error when write on close fails #1118

Merged
merged 1 commit into from
Sep 5, 2019

Conversation

mstoykov
Copy link
Collaborator

No description provided.

@mstoykov
Copy link
Collaborator Author

I am expecting that this is the root cause of an issue reported in the community forum, as this is the only place where a nil error can be send :)

I would've added tests but we are using some service to test websockets and I didn't find a way to have a write error with it in the 5 minutes I looked into it ... so maybe adding some priority to #537 and specifically the part with websockets ... it truly shouldn't be that hard to have something like httpbin for it

@codecov-io
Copy link

codecov-io commented Aug 23, 2019

Codecov Report

Merging #1118 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1118      +/-   ##
==========================================
+ Coverage   73.54%   73.59%   +0.05%     
==========================================
  Files         144      144              
  Lines       10531    10530       -1     
==========================================
+ Hits         7745     7750       +5     
+ Misses       2327     2322       -5     
+ Partials      459      458       -1
Impacted Files Coverage Δ
js/modules/k6/ws/ws.go 81.3% <100%> (+2.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e438a1...149020c. Read the comment docs.

cuonglm
cuonglm previously approved these changes Aug 23, 2019
imiric
imiric previously approved these changes Aug 23, 2019
@na-- na-- added this to the v0.26.0 milestone Aug 29, 2019
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

We already have websocket tests in the httpmultibin, so it shouldn't be too difficult to add a test for this? https://github.com/loadimpact/k6/blob/f2dff90edc83925399e738195e5617eaf7145398/lib/testutils/httpmultibin.go#L172-L173

Also, please write a PR description suitable for the release notes

@mstoykov mstoykov merged commit 38ef05c into master Sep 5, 2019
@mstoykov mstoykov deleted the fixWSReportingWrongError branch September 5, 2019 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants