Add cookie to ws #1650

lcd1232 · 2020-10-04T12:52:26Z

Closes #1226.
Add header Cookie which allows sending cookies to WebSocket.

This reverts commit 8d5e6d5

codecov-commenter · 2020-10-04T13:04:52Z

Codecov Report

Merging #1650 into master will decrease coverage by 0.05%.
The diff coverage is 13.33%.

@@            Coverage Diff             @@
##           master    #1650      +/-   ##
==========================================
- Coverage   72.16%   72.11%   -0.06%     
==========================================
  Files         166      166              
  Lines       12787    12802      +15     
==========================================
+ Hits         9228     9232       +4     
- Misses       3009     3020      +11     
  Partials      550      550

Impacted Files	Coverage Δ
js/modules/k6/ws/ws.go	`77.69% <13.33%> (-3.81%)`	⬇️
lib/testutils/minirunner/minirunner.go	`81.39% <0.00%> (-4.66%)`	⬇️
stats/cloud/collector.go	`80.44% <0.00%> (+0.63%)`	⬆️
lib/executor/vu_handle.go	`95.23% <0.00%> (+1.90%)`	⬆️

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 9b75efd...29039b1. Read the comment docs.

imiric

Thanks for your contribution! I left some minor comments, but we should also have a unit test or two for this change.

js/modules/k6/ws/ws.go

codecov-io · 2020-10-11T11:29:43Z

Codecov Report

Merging #1650 into master will decrease coverage by 0.03%.
The diff coverage is 61.40%.

@@            Coverage Diff             @@
##           master    #1650      +/-   ##
==========================================
- Coverage   72.16%   72.13%   -0.04%     
==========================================
  Files         166      166              
  Lines       12787    12840      +53     
==========================================
+ Hits         9228     9262      +34     
- Misses       3009     3022      +13     
- Partials      550      556       +6

Impacted Files	Coverage Δ
js/modules/k6/ws/ws.go	`77.74% <57.44%> (-3.76%)`	⬇️
lib/testutils/httpmultibin/httpmultibin.go	`91.46% <80.00%> (-0.95%)`	⬇️
lib/executor/vu_handle.go	`96.19% <0.00%> (+2.85%)`	⬆️

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 9b75efd...07b3846. Read the comment docs.

lcd1232 · 2020-10-11T12:20:25Z

@imiric hello. I rewrite it to use cookie object

codecov-commenter · 2020-10-11T12:27:58Z

Codecov Report

Merging #1650 into master will decrease coverage by 0.03%.
The diff coverage is 61.40%.

@@            Coverage Diff             @@
##           master    #1650      +/-   ##
==========================================
- Coverage   72.16%   72.13%   -0.04%     
==========================================
  Files         166      166              
  Lines       12787    12840      +53     
==========================================
+ Hits         9228     9262      +34     
- Misses       3009     3022      +13     
- Partials      550      556       +6

Impacted Files	Coverage Δ
js/modules/k6/ws/ws.go	`77.74% <57.44%> (-3.76%)`	⬇️
lib/testutils/httpmultibin/httpmultibin.go	`91.46% <80.00%> (-0.95%)`	⬇️
lib/executor/vu_handle.go	`96.19% <0.00%> (+2.85%)`	⬆️

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 9b75efd...07b3846. Read the comment docs.

codecov-io · 2020-10-11T12:29:27Z

Codecov Report

Merging #1650 into master will decrease coverage by 0.00%.
The diff coverage is 61.40%.

@@            Coverage Diff             @@
##           master    #1650      +/-   ##
==========================================
- Coverage   72.16%   72.15%   -0.01%     
==========================================
  Files         166      166              
  Lines       12787    12840      +53     
==========================================
+ Hits         9228     9265      +37     
- Misses       3009     3019      +10     
- Partials      550      556       +6

Impacted Files	Coverage Δ
js/modules/k6/ws/ws.go	`77.74% <57.44%> (-3.76%)`	⬇️
lib/testutils/httpmultibin/httpmultibin.go	`91.46% <80.00%> (-0.95%)`	⬇️
stats/cloud/collector.go	`80.44% <0.00%> (+0.63%)`	⬆️
js/runner.go	`83.01% <0.00%> (+0.75%)`	⬆️
lib/executor/vu_handle.go	`95.23% <0.00%> (+1.90%)`	⬆️

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 9b75efd...07b3846. Read the comment docs.

lcd1232 · 2020-10-19T06:32:45Z

@imiric @mstoykov ping

js/modules/k6/ws/ws.go

imiric · 2020-10-19T10:36:15Z

js/modules/k6/ws/ws.go

+	}
+
+	cookiesMap := make(map[string]http.Cookie)
+	for _, cookie := range state.CookieJar.Cookies(u) {


Should we write the cookiesMap changes back to state.CookieJar? cc: @mstoykov

And I think @mstoykov would like to reuse the cookie logic in the httpext package, e.g. SetRequestCookies, though we should probably agree on the details.

Looking at the websocket docs it looks like reusing it will be ... harder than I expected ... read ... not possible unless we rewrite it which won't help at all either way.

Looking at the httpext k6 does write back only stuff that were set in the responses

imiric · 2020-10-19T10:41:04Z

Thanks for the changes @lcd1232. Would you mind rebasing this branch on the current master, since we switched CI to GitHub Actions?

imiric

@lcd1232 Are you still available to wrap up this PR? We'd like to merge it for v0.33.0 as the code LGTM besides the test comment, but it would need rebasing since there have been a lot of changes in the past 7 months (though few related to WS, so there are no conflicts), and you'll need to change the test to use rt.RunString() instead of common.RunString() since we dropped our wrapper.

If we don't hear from you in the next few days or you're not available anymore, that's OK too, and we can continue and merge it ourselves. You would of course be credited in our release notes ;)

imiric · 2021-05-20T15:51:05Z

js/modules/k6/ws/ws_test.go

+	t.Run("client_cookie", func(t *testing.T) {
+		_, err := common.RunString(rt, sr(`
+		var params = {
+			cookies: { "Session": "123" },


We should also have a test for when the value is an object and uses replace.

mstoykov · 2021-10-26T08:03:28Z

Given that there is both a newer PR #2052 and with #2193 I actually fixed the original issue, I am closing this.

Thanks for all the work @lcd1232, even if we didn't merge this it helped push it forward 🙇

lcd1232 added 5 commits October 4, 2020 15:28

Add StatusText field

8d5e6d5

Revert "Add StatusText field"

4878f93

This reverts commit 8d5e6d5

Add X-Cookie header for cookies

510e9de

Rename X-Cookie to Cookie

d5fb3d0

Fix linter

29039b1

imiric suggested changes Oct 5, 2020

View reviewed changes

js/modules/k6/ws/ws.go Outdated Show resolved Hide resolved

js/modules/k6/ws/ws.go Outdated Show resolved Hide resolved

Add test

8b39007

lcd1232 added 4 commits October 11, 2020 14:36

Add merge logic for cookies

e536867

Fix linter

ee3d37e

Fix error

1d05157

Improve code

4ed47e5

Remove unused variable

07b3846

imiric reviewed Oct 19, 2020

View reviewed changes

js/modules/k6/ws/ws.go Show resolved Hide resolved

imiric reviewed Oct 19, 2020

View reviewed changes

mstoykov added this to the v0.30.0 milestone Nov 4, 2020

na-- modified the milestones: v0.30.0, v0.31.0 Jan 13, 2021

na-- modified the milestones: v0.31.0, v0.32.0 Mar 5, 2021

This was referenced Mar 23, 2021

Wrong cookie merging #996

Closed

HTTP Cookies merge fix proposal #1923

Open

na-- modified the milestones: v0.32.0, v0.33.0 Apr 14, 2021

imiric suggested changes May 20, 2021

View reviewed changes

imiric mentioned this pull request Jun 8, 2021

Add cookie support to WebSockets #2052

Closed

na-- modified the milestones: v0.33.0, v0.34.0 Jun 16, 2021

mstoykov modified the milestones: v0.34.0, v0.35.0 Sep 1, 2021

mstoykov closed this Oct 26, 2021

lcd1232 deleted the add_cookie_to_ws branch October 26, 2021 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cookie to ws #1650

Add cookie to ws #1650

lcd1232 commented Oct 4, 2020

codecov-commenter commented Oct 4, 2020 •

edited

Loading

imiric left a comment

codecov-io commented Oct 11, 2020 •

edited

Loading

lcd1232 commented Oct 11, 2020

codecov-commenter commented Oct 11, 2020

codecov-io commented Oct 11, 2020 •

edited

Loading

lcd1232 commented Oct 19, 2020

imiric Oct 19, 2020

mstoykov Oct 19, 2020

imiric commented Oct 19, 2020

imiric left a comment

imiric May 20, 2021

mstoykov commented Oct 26, 2021

Add cookie to ws #1650

Add cookie to ws #1650

Conversation

lcd1232 commented Oct 4, 2020

codecov-commenter commented Oct 4, 2020 • edited Loading

Codecov Report

imiric left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 11, 2020 • edited Loading

Codecov Report

lcd1232 commented Oct 11, 2020

codecov-commenter commented Oct 11, 2020

Codecov Report

codecov-io commented Oct 11, 2020 • edited Loading

Codecov Report

lcd1232 commented Oct 19, 2020

imiric Oct 19, 2020

Choose a reason for hiding this comment

mstoykov Oct 19, 2020

Choose a reason for hiding this comment

imiric commented Oct 19, 2020

imiric left a comment

Choose a reason for hiding this comment

imiric May 20, 2021

Choose a reason for hiding this comment

mstoykov commented Oct 26, 2021

codecov-commenter commented Oct 4, 2020 •

edited

Loading

codecov-io commented Oct 11, 2020 •

edited

Loading

codecov-io commented Oct 11, 2020 •

edited

Loading