More multi message tests for WebSockets #2184

cooliscool · 2021-10-13T16:58:52Z

In relation to #1152

Explanation :

Didn't modify the ws-echo handler in httpmultibin, instead I extended httpmultibin with a tb.Mux.HandleFunc(...)

Here are the few tests I could add:

send_receive_multiple_ws
send and receive messages 'test1', 'test2', 'test3' in sequence then close.
send_receive_multiple_wss
send and receive messages 'test1', and then 'test2', then close on a wss connection
send_receive_text_binary
I thought of this scenario where multiple kind of messages (text and binary) are exchanged over the same socket. So, here's a test that sends a text message and a binary message in sequence.

Other than that, I'm a bit concerned about whether running these tests parallely using same runtime will cause a race condition, for test to fail. (randomly, only if some race succeeds)

And, please let me know if any of these tests needs to be modified, or if you'd like me to add more scenarios, or remove.

ALso, I've added a funtion assertMetricEmittedCount() for asserting the count of ws_msgs_sent and ws_msgs_received collected. I made the number of messages sent '3' in send_receive_multiple_ws because of unparam linter complaining about passing same value to assertMetricEmittedCount() always :/ but that extra param count could be useful later.

codecov-commenter · 2021-10-13T17:14:40Z

Codecov Report

Merging #2184 (4cffcbe) into master (42f2e60) will increase coverage by 0.03%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #2184      +/-   ##
==========================================
+ Coverage   72.71%   72.75%   +0.03%     
==========================================
  Files         184      184              
  Lines       14571    14577       +6     
==========================================
+ Hits        10596    10605       +9     
+ Misses       3333     3330       -3     
  Partials      642      642

Flag	Coverage Δ
ubuntu	`72.68% <71.42%> (+0.03%)`	⬆️
windows	`72.39% <71.42%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/login_cloud.go	`0.00% <0.00%> (ø)`
cmd/login_influxdb.go	`0.00% <0.00%> (ø)`
cmd/ui.go	`20.61% <0.00%> (ø)`
js/compiler/compiler.go	`55.00% <ø> (ø)`
lib/execution_segment.go	`92.73% <ø> (ø)`
lib/executor/ramping_vus.go	`94.66% <ø> (ø)`
lib/metrics/metrics.go	`100.00% <ø> (ø)`
ui/form_fields.go	`45.71% <0.00%> (ø)`
js/modules/k6/ws/ws.go	`83.51% <100.00%> (+0.35%)`	⬆️
output/csv/config.go	`74.00% <100.00%> (+4.23%)`	⬆️
... and 2 more

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 f7a1ba1...4cffcbe. Read the comment docs.

imiric

Thanks for the PR @cooliscool! This LGTM, just noticed a typo in a test that doesn't really change the outcome.

I'm a bit concerned about whether running these tests parallely using same runtime will cause a race condition, for test to fail. (randomly, only if some race succeeds)

Yeah, we can't run the subtests in parallel because of the shared goja.Runtime, and while the current t.Parallel() you have in the multi_message test doesn't do any harm, I would prefer if it was removed to be consistent with the other TestSession tests. We plan to overhaul the WS module in the future and fixing these tests can be done then.

js/modules/k6/ws/ws_test.go

imiric

LGTM, thanks!

I wouldn't be against changing the ws-echo HTTPMultiBin handler to support multiple messages, since it's used by at least 3 tests now, but adding it in the test is fine as well.

yorugac

Thank you! LGTM overall, just one thing that I think makes sense to check. There is now assertMetricEmittedCount() and assertMetricEmitted(): would all tests pass if you substitute assertMetricEmitted with assertMetricEmittedCount and count = 1? By brief look it should work just as well and then we won't increase the number of helper functions there.

cooliscool · 2021-10-15T13:34:52Z

sorry. closed this by mistake.

let me summarise the suggested changes here :

replace instances of assertMetricEmitted() with assertMetricEmittedCount() and count = 1 to reduce the no. of helper functions.
changing the ws-echo HTTPMultiBin handler to support multiple messages. instead of using a test specific handler. the gotcha here would be that existing tests depending on ws-echo shouldn't break.

Its doable without making huge changes in the existing tests i believe.

imiric · 2021-10-15T13:45:26Z

@cooliscool Don't worry about the HTTPMultiBin change, it's probably better that this uses an isolated handler. And we can move it later if we need it elsewhere.

inancgumus

Nice work, thanks. I have some suggestions.

inancgumus · 2021-10-15T13:38:29Z

js/modules/k6/ws/ws_test.go

+			assert.True(t, ok)
+			if surl == url {
+				if sample.Metric.Name == metricName {
+					actualCount++


What about doing this like:

if surl != url && sample.Metric.Name != metricName { continue } actualCount++

Imho, our eyes are pretty good at following lines; left-indentation can improve glancing at a piece of code.

wouldn't this be more straightforward?

if surl == url && sample.Metric.Name == metricName { actualCount++ }

instead of

if surl == url { if sample.Metric.Name == metricName { actualCount++ } }

@inancgumus

inancgumus · 2021-10-15T13:50:04Z

js/modules/k6/ws/ws_test.go

+					socket.send(msg1)
+				})
+				socket.on("message", function (data){
+					if (data == msg1){


I suggest doing:

socket.on("message", function (data){ -> socket.on("message", function (data) {

The same, also, for the if statements below.

inancgumus · 2021-10-15T13:50:47Z

js/modules/k6/ws/ws_test.go

+			});
+
+			if (!thirdMsgRecd) {
+				throw new Error ("third test message was not received!");


Can you add an error why the third message was not received? So, we can see what to look for on an error.

inancgumus · 2021-10-15T13:54:44Z

js/modules/k6/ws/ws_test.go

@@ -88,6 +89,23 @@ func assertMetricEmitted(t *testing.T, metricName string, sampleContainers []sta
 	assert.True(t, seenMetric, "url %s didn't emit %s", url, metricName)
 }

+func assertMetricEmittedCount(t *testing.T, metricName string, sampleContainers []stats.SampleContainer, url string, count int) {
+	actualCount := 0


Can you make this helper function a t.Helper?

js/modules/k6/ws/ws_test.go

inancgumus · 2021-10-15T13:57:58Z

js/modules/k6/ws/ws_test.go

@@ -88,6 +89,23 @@ func assertMetricEmitted(t *testing.T, metricName string, sampleContainers []sta
 	assert.True(t, seenMetric, "url %s didn't emit %s", url, metricName)
 }

+func assertMetricEmittedCount(t *testing.T, metricName string, sampleContainers []stats.SampleContainer, url string, count int) {


Maybe we should make this function accept a struct. It has a lot of parameters, and, IMHO, it makes it hard to follow what's going on when reading the usage of this function inside the tests.

yorugac · 2021-10-15T14:22:16Z

1. replace instances of `assertMetricEmitted()` with `assertMetricEmittedCount()` and count = 1 to reduce the no. of helper functions.

@cooliscool Yes please about this ^^. If it works, then it's best to have a simplified version.

And I also agree with @imiric on isolated handler being better at this point 👍

inancgumus

LGTM, thanks for your contribution. 🎉

yorugac

LGTM @cooliscool, thank you 🙂

cooliscool · 2021-10-19T14:27:30Z

@imiric @inancgumus @yorugac thanks for the review. ♥

cooliscool added 3 commits October 13, 2021 21:51

add multi msg tests for ws

3814ceb

Merge branch 'grafana:master' into wsMoreTests#1152

d4b6c9d

added more assertions

3d809dc

github-actions bot requested review from imiric and yorugac October 13, 2021 16:59

imiric suggested changes Oct 14, 2021

View reviewed changes

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

fixed typo

bbbd05c

cooliscool requested a review from imiric October 14, 2021 13:08

imiric previously approved these changes Oct 15, 2021

View reviewed changes

imiric linked an issue Oct 15, 2021 that may be closed by this pull request

Create multi-message WebSockets test #1152

Closed

yorugac reviewed Oct 15, 2021

View reviewed changes

cooliscool closed this Oct 15, 2021

cooliscool reopened this Oct 15, 2021

github-actions bot requested review from inancgumus and na-- October 15, 2021 13:25

imiric removed request for inancgumus and na-- October 15, 2021 13:45

inancgumus requested changes Oct 15, 2021

View reviewed changes

cooliscool added 2 commits October 19, 2021 13:11

Merge branch 'grafana:master' into wsMoreTests#1152

4cffcbe

refactoring according to suggestions

1dcb685

cooliscool dismissed imiric’s stale review via 1dcb685 October 19, 2021 08:57

cooliscool requested review from inancgumus, imiric and yorugac October 19, 2021 08:58

small refactoring

f46f112

inancgumus approved these changes Oct 19, 2021

View reviewed changes

yorugac approved these changes Oct 19, 2021

View reviewed changes

yorugac merged commit aab12d5 into grafana:master Oct 19, 2021

na-- added this to the v0.35.0 milestone Oct 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More multi message tests for WebSockets #2184

More multi message tests for WebSockets #2184

cooliscool commented Oct 13, 2021

codecov-commenter commented Oct 13, 2021 •

edited

Loading

imiric left a comment

imiric left a comment

yorugac left a comment

cooliscool commented Oct 15, 2021

imiric commented Oct 15, 2021

inancgumus left a comment

inancgumus Oct 15, 2021

cooliscool Oct 19, 2021 •

edited

Loading

inancgumus Oct 15, 2021

inancgumus Oct 15, 2021

inancgumus Oct 15, 2021

inancgumus Oct 15, 2021

yorugac commented Oct 15, 2021

inancgumus left a comment

yorugac left a comment

cooliscool commented Oct 19, 2021

More multi message tests for WebSockets #2184

More multi message tests for WebSockets #2184

Conversation

cooliscool commented Oct 13, 2021

Explanation :

codecov-commenter commented Oct 13, 2021 • edited Loading

Codecov Report

imiric left a comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

yorugac left a comment

Choose a reason for hiding this comment

cooliscool commented Oct 15, 2021

imiric commented Oct 15, 2021

inancgumus left a comment

Choose a reason for hiding this comment

inancgumus Oct 15, 2021

Choose a reason for hiding this comment

cooliscool Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

inancgumus Oct 15, 2021

Choose a reason for hiding this comment

inancgumus Oct 15, 2021

Choose a reason for hiding this comment

inancgumus Oct 15, 2021

Choose a reason for hiding this comment

inancgumus Oct 15, 2021

Choose a reason for hiding this comment

yorugac commented Oct 15, 2021

inancgumus left a comment

Choose a reason for hiding this comment

yorugac left a comment

Choose a reason for hiding this comment

cooliscool commented Oct 19, 2021

codecov-commenter commented Oct 13, 2021 •

edited

Loading

cooliscool Oct 19, 2021 •

edited

Loading