Upgrade xk6-browser to v1.0.2 #3235

ka3de · 2023-07-28T09:02:41Z

What?

This PR upgrades the xk6-browser extension version to v1.0.2. Being this one the latest release for the extension.

Additionally, it also removes the requirement for the K6_BROWSER_ENABLED environment variable in order to execute tests that use the experimental browser module.

Why?

Upgrade
Keep xk6-browser extension version up to date in k6.

Removal of K6_BROWSER_ENABLED flag
Currently (as of xk6-browser v1.0.2) the browser extension requires the definition of the browser type parameter inside the options element for every scenario that wants to use the browser module. Therefore the usage of K6_BROWSER_ENABLED flag as an explicit approval to use the browser module has become redundant, as both parameters have to be set, being the scenario one more restrictive.

Checklist

I have performed a self-review of my code.
I have added tests for my changes. (N/A)
I have run linter locally (make ci-like-lint) and all checks pass.
I have run tests locally (make tests) and all tests pass.
I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #3197

codecov-commenter · 2023-07-28T09:13:33Z

Codecov Report

Merging #3235 (bfcd055) into master (fcb19fb) will decrease coverage by 0.18%.
The diff coverage is n/a.

❗ Current head bfcd055 differs from pull request most recent head 0837ee5. Consider uploading reports for the commit 0837ee5 to get more accurate results

@@            Coverage Diff             @@
##           master    #3235      +/-   ##
==========================================
- Coverage   73.22%   73.05%   -0.18%     
==========================================
  Files         259      256       -3     
  Lines       19899    19882      -17     
==========================================
- Hits        14572    14524      -48     
- Misses       4403     4426      +23     
- Partials      924      932       +8

Flag	Coverage Δ
ubuntu	`?`
windows	`73.05% <ø> (-0.03%)`	⬇️

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

Files Changed	Coverage Δ
js/jsmodules.go	`100.00% <ø> (ø)`

... and 12 files with indirect coverage changes

mstoykov · 2023-07-28T09:36:11Z

cmd/tests/cmd_run_test.go

@@ -1827,80 +1827,6 @@ func BenchmarkReadResponseBody(b *testing.B) {
 	cmd.ExecuteWithGlobalState(ts.GlobalState)
 }

-func TestBrowserPermissions(t *testing.T) {


Can we actually have a test with the scenarios options instead?

+1 for having an e2e coverage

I've tried to create a test but now I think i've found an issue with the events channel from the new event loop that the browser module subscribes to.

After creating a simple test

Simple test

func TestBrowserPermissions(t *testing.T) { t.Parallel() tests := []struct { name string expectedExitCode exitcodes.ExitCode expectedError string }{ { name: "browser option not set", expectedExitCode: 0, expectedError: "GoError: browser not found in registry. make sure to set browser type option in scenario definition in order to use the browser module", }, } for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() script := ` import { browser } from 'k6/experimental/browser'; export default function() { browser.isConnected(); }; ` ts := getSingleFileTestState(t, script, []string{}, tt.expectedExitCode) cmd.ExecuteWithGlobalState(ts.GlobalState) loglines := ts.LoggerHook.Drain() if tt.expectedError == "" { require.Len(t, loglines, 0) return } assert.Contains(t, loglines[0].Message, tt.expectedError) }) } }

The output of the test is:

found unexpected goroutines: [Goroutine 69 in state chan receive, with github.com/grafana/xk6-browser/browser.(*browserRegistry).handleIterEvents on top of the stack: goroutine 69 [chan receive]: github.com/grafana/xk6-browser/browser.(*browserRegistry).handleIterEvents(0x14001fb6b00, 0x0?, 0x14000b7f6e0) /Users/ankuragarwal/go/src/github.com/grafana/k6/vendor/github.com/grafana/xk6-browser/browser/registry.go:239 +0xa4 created by github.com/grafana/xk6-browser/browser.newBrowserRegistry /Users/ankuragarwal/go/src/github.com/grafana/k6/vendor/github.com/grafana/xk6-browser/browser/registry.go:226 +0x2f8 Goroutine 60 in state chan receive, with github.com/grafana/xk6-browser/browser.(*browserRegistry).handleIterEvents on top of the stack: goroutine 60 [chan receive]: github.com/grafana/xk6-browser/browser.(*browserRegistry).handleIterEvents(0x14001fd90c0, 0x105b12700?, 0x14000d0de30) /Users/ankuragarwal/go/src/github.com/grafana/k6/vendor/github.com/grafana/xk6-browser/browser/registry.go:239 +0xa4 created by github.com/grafana/xk6-browser/browser.newBrowserRegistry /Users/ankuragarwal/go/src/github.com/grafana/k6/vendor/github.com/grafana/xk6-browser/browser/registry.go:226 +0x2f8 ]

So it would seem that the channel the browser module gets after subscribing isn't being closed when the test exits, am i correct in thinking that? I'd prefer to suppress this error for now and work on the fix in the next cycle.

WDYT?

isn't being closed when the test exits, am i correct in thinking that?

Yes, it seems you're right. Do you have a guess why this is happening? I see the code for unsubscribing is implemented on the core and invoked from the browser.

I'd prefer to suppress this error for now and work on the fix in the next cycle.

How do you intend to suppress it? Do you want to remove the e2e test? I would prefer if at least we understand the root of the problem and we open an issue so we can classify it as non-critical with confidence.

Ok, I'll take a look.

I've created a new issue. I believe the issue actually lies within the browser module scope and so we will work on it asap.

When the test run completes, does the k6 processes exit, or can it ever be reused? The reason I'm asking is because these uncleared goroutines will leak if the k6 process is reused. If it isn't then this is non-critical issue, but we will not be able to add the e2e test in this release.

We have a fix for the leaky goroutines in grafana/xk6-browser#990. As soon as that has been merged into main, i'll upgrade this PR to work with the latest commit from main, and finally add the e2e tests for this PR.

I've updated this branch to work with v1.0.1 of xk6-browser which contains the fix for the goroutine leaks. I've also added an e2e test.

It's worth noting that the e2e test will start a chrome process in the background.

Also, i'm looking at this data race now 😓

This has now been resolved in a32cc6f. Could you please take a look @mstoykov and @codebien?

mstoykov

I would still prefer to have a test that the options enable the browser and without them it won't work.

js/jsmodules.go

codebien · 2023-07-31T12:43:33Z

cmd/tests/cmd_run_test.go

@@ -1827,80 +1827,6 @@ func BenchmarkReadResponseBody(b *testing.B) {
 	cmd.ExecuteWithGlobalState(ts.GlobalState)
 }

-func TestBrowserPermissions(t *testing.T) {


+1 for having an e2e coverage

Resolves: #3235 (comment)

andrewslotin

@ka3de, could you also drop K6_BROWSER_ENABLED from the Dockerfile?

ka3de · 2023-08-07T11:35:45Z

@ka3de, could you also drop K6_BROWSER_ENABLED from the Dockerfile?

Done in 85af866.

Currently (as of grafana/xk6-browser@v1.0.0) the browser extension requires the definition of the browser type parameter inside the options element for every scenario that wants to use the browser module. Therefore the K6_BROWSER_ENABLED flag has become redundant, as both parameters have to be set, being the scenario one more restrictive. Because we no longer have to parse the environment variable, the module wrapper implementation can be removed completely and use the xk6-browser root module constructor directly instead.

Resolves: #3235 (comment)

This includes some fixes for the goroutine leaks that we were seeing when we tried to create the e2e tests for the browser module in k6.

This test ensures that an error occurs and the test ends when the browser module is imported but the options are missing, otherwise when setup correctly the browser test runs.

This contains a fix for the race condition where the ctx was being read before the vu was setup.

github-actions bot requested review from imiric and olegbespalov July 28, 2023 09:02

ka3de mentioned this pull request Jul 28, 2023

Remove K6_BROWSER_ENABLED flag requirement #3221

Closed

5 tasks

ka3de force-pushed the upgrade/k6-latest-1-0-0 branch from 6184d29 to 8a204fc Compare July 28, 2023 09:34

mstoykov requested review from mstoykov and oleiade July 28, 2023 09:34

mstoykov reviewed Jul 28, 2023

View reviewed changes

mstoykov previously approved these changes Jul 28, 2023

View reviewed changes

ka3de requested review from inancgumus and ankur22 July 28, 2023 11:00

inancgumus previously approved these changes Jul 28, 2023

View reviewed changes

oleiade previously approved these changes Jul 28, 2023

View reviewed changes

mstoykov added this to the v0.46.0 milestone Jul 31, 2023

codebien reviewed Jul 31, 2023

View reviewed changes

ankur22 added a commit that referenced this pull request Aug 1, 2023

Use browser package name instead of expBrowser

8e31247

Resolves: #3235 (comment)

ankur22 dismissed stale reviews from oleiade, inancgumus, and mstoykov via 8e31247 August 1, 2023 11:30

This was referenced Aug 2, 2023

handleIterEvents goroutines not being cleared up grafana/xk6-browser#989

Closed

Fix lingering/leaking goroutines for k6 upgrade grafana/xk6-browser#990

Merged

ankur22 changed the title ~~Upgrade xk6-browser to v1.0.0~~ Upgrade xk6-browser to v1.0.1 Aug 3, 2023

ankur22 requested review from codebien and mstoykov August 3, 2023 13:10

andrewslotin reviewed Aug 7, 2023

View reviewed changes

andrewslotin previously approved these changes Aug 7, 2023

View reviewed changes

inancgumus previously approved these changes Aug 7, 2023

View reviewed changes

ka3de added 2 commits August 7, 2023 16:59

Upgrade xk6-browser to v1.0.0

90ac2f9

ankur22 added 3 commits August 7, 2023 16:59

Use browser package name instead of expBrowser

b02dcd2

Resolves: #3235 (comment)

Update to v1.0.1 of xk6-browser

b1fbe53

This includes some fixes for the goroutine leaks that we were seeing when we tried to create the e2e tests for the browser module in k6.

Add e2e test for browser module in k6

5eee08d

This test ensures that an error occurs and the test ends when the browser module is imported but the options are missing, otherwise when setup correctly the browser test runs.

ankur22 dismissed stale reviews from inancgumus and andrewslotin via 5eee08d August 7, 2023 16:00

ankur22 force-pushed the upgrade/k6-latest-1-0-0 branch from 85af866 to 5eee08d Compare August 7, 2023 16:00

ankur22 requested review from andrewslotin, inancgumus and oleiade August 8, 2023 08:46

ankur22 changed the title ~~Upgrade xk6-browser to v1.0.1~~ Upgrade xk6-browser to v1.0.2 Aug 8, 2023

Update to v1.0.2 of xk6-browser

0837ee5

This contains a fix for the race condition where the ctx was being read before the vu was setup.

ankur22 force-pushed the upgrade/k6-latest-1-0-0 branch from a32cc6f to 0837ee5 Compare August 8, 2023 08:57

mstoykov approved these changes Aug 9, 2023

View reviewed changes

codebien approved these changes Aug 9, 2023

View reviewed changes

codebien merged commit 9598ef5 into master Aug 9, 2023
22 checks passed

codebien deleted the upgrade/k6-latest-1-0-0 branch August 9, 2023 07:41

codebien mentioned this pull request Sep 6, 2023

dockerfile: Drop K6_BROWSER_ENABLED env #3324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade xk6-browser to v1.0.2 #3235

Upgrade xk6-browser to v1.0.2 #3235

ka3de commented Jul 28, 2023 •

edited by ankur22

Loading

codecov-commenter commented Jul 28, 2023 •

edited

Loading

mstoykov Jul 28, 2023

codebien Jul 31, 2023

ankur22 Aug 1, 2023

codebien Aug 1, 2023

ankur22 Aug 1, 2023

ankur22 Aug 2, 2023

ankur22 Aug 2, 2023 •

edited

Loading

ankur22 Aug 3, 2023 •

edited

Loading

ankur22 Aug 8, 2023

mstoykov left a comment

codebien Jul 31, 2023

andrewslotin left a comment

ka3de commented Aug 7, 2023

Upgrade xk6-browser to v1.0.2 #3235

Upgrade xk6-browser to v1.0.2 #3235

Conversation

ka3de commented Jul 28, 2023 • edited by ankur22 Loading

What?

Why?

Checklist

Related PR(s)/Issue(s)

codecov-commenter commented Jul 28, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankur22 Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

ankur22 Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewslotin left a comment

Choose a reason for hiding this comment

ka3de commented Aug 7, 2023

ka3de commented Jul 28, 2023 •

edited by ankur22

Loading

codecov-commenter commented Jul 28, 2023 •

edited

Loading

ankur22 Aug 2, 2023 •

edited

Loading

ankur22 Aug 3, 2023 •

edited

Loading