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.

Sign up for GitHub

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

Upgrade xk6-browser to v1.0.2 #3235

Merged
merged 6 commits into from
Aug 9, 2023
Merged

Upgrade xk6-browser to v1.0.2 #3235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
90ac2f9
Upgrade xk6-browser to v1.0.0
ka3de Jul 28, 2023
9e09acb
Remove K6_BROWSER_ENABLED flag requirement
ka3de Jul 21, 2023
b02dcd2
Use browser package name instead of expBrowser
ankur22 Aug 1, 2023
b1fbe53
Update to v1.0.1 of xk6-browser
ankur22 Aug 3, 2023
5eee08d
Add e2e test for browser module in k6
ankur22 Aug 3, 2023
0837ee5
Update to v1.0.2 of xk6-browser
ankur22 Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 62 additions & 74 deletions cmd/tests/cmd_run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1827,80 +1827,6 @@ func BenchmarkReadResponseBody(b *testing.B) {
cmd.ExecuteWithGlobalState(ts.GlobalState)
}

func TestBrowserPermissions(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for having an e2e coverage

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll take a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ankur22 ankur22 Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ankur22 ankur22 Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 😓

Copy link
Contributor

Choose a reason for hiding this comment

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

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

t.Parallel()

tests := []struct {
name string
envVarValue string
envVarMsgValue string
expectedExitCode exitcodes.ExitCode
expectedError string
}{
{
name: "no env var set",
envVarValue: "",
expectedExitCode: 107,
expectedError: "To run browser tests set env var K6_BROWSER_ENABLED=true",
},
{
name: "env var set but set to false",
envVarValue: "false",
expectedExitCode: 107,
expectedError: "To run browser tests set env var K6_BROWSER_ENABLED=true",
},
{
name: "env var set but set to 09adsu",
envVarValue: "09adsu",
expectedExitCode: 107,
expectedError: "To run browser tests set env var K6_BROWSER_ENABLED=true",
},
{
name: "with custom message",
envVarValue: "09adsu",
envVarMsgValue: "Try again later",
expectedExitCode: 107,
expectedError: "Try again later",
},
{
name: "env var set and set to true",
envVarValue: "true",
expectedExitCode: 0,
expectedError: "",
},
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
script := `
import { chromium } from 'k6/experimental/browser';

export default function() {};
`

ts := getSingleFileTestState(t, script, []string{}, tt.expectedExitCode)
if tt.envVarValue != "" {
ts.Env["K6_BROWSER_ENABLED"] = tt.envVarValue
}
if tt.envVarMsgValue != "" {
ts.Env["K6_BROWSER_ENABLED_MSG"] = tt.envVarMsgValue
}
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)
})
}
}

func TestUIRenderOutput(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -2228,3 +2154,65 @@ func BenchmarkRunEvents(b *testing.B) {
}
}
}

func TestBrowserPermissions(t *testing.T) {
t.Parallel()

tests := []struct {
name string
options string
expectedExitCode exitcodes.ExitCode
expectedError string
}{
{
name: "browser option not set",
options: "",
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",
},
{
name: "browser option set",
options: `export const options = {
scenarios: {
browser: {
executor: 'shared-iterations',
options: {
browser: {
type: 'chromium',
},
},
},
},
}`,
expectedExitCode: 0,
expectedError: "",
},
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
script := fmt.Sprintf(`
import { browser } from 'k6/experimental/browser';

%s

export default function() {
browser.isConnected();
};
`, tt.options)

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)
})
}
}
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ require (
github.com/go-sourcemap/sourcemap v2.1.4-0.20211119122758-180fcef48034+incompatible
github.com/golang/protobuf v1.5.3
github.com/gorilla/websocket v1.5.0
github.com/grafana/xk6-browser v0.10.0
github.com/grafana/xk6-browser v1.0.2
github.com/grafana/xk6-grpc v0.1.3-0.20230717090346-fb49221e0ce1
github.com/grafana/xk6-output-prometheus-remote v0.2.3
github.com/grafana/xk6-redis v0.1.1
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,8 @@ github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+
github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk=
github.com/gorilla/websocket v1.5.0 h1:PPwGk2jz7EePpoHN/+ClbZu8SPxiqlu12wZP/3sWmnc=
github.com/gorilla/websocket v1.5.0/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE=
github.com/grafana/xk6-browser v0.10.0 h1:Mnx0Ho+mlyFGlV7zW7zXkN0njRglh9JflLV+OzXSaRk=
github.com/grafana/xk6-browser v0.10.0/go.mod h1:ax6OHARpNEu9hSGYOAI4grAwiRapsNPi9TBQxDYurKw=
github.com/grafana/xk6-browser v1.0.2 h1:B9ll8xLH68hfCBy3sTzhmksCxwgJBIcqgPeX3mht6jM=
github.com/grafana/xk6-browser v1.0.2/go.mod h1:LV/ECGBCN3vRN/A4St+Ep9JUpbKJuRsj+6TBihQptGw=
github.com/grafana/xk6-grpc v0.1.3-0.20230717090346-fb49221e0ce1 h1:SdMihJN+fkH6cO/1NeAnVxSVOnJ3ZkZ1v7FJnrcqhog=
github.com/grafana/xk6-grpc v0.1.3-0.20230717090346-fb49221e0ce1/go.mod h1:iq6qHN64XgAEmDHKf0OXZ4mvoqF4Udr22fiCIXNpXA0=
github.com/grafana/xk6-output-prometheus-remote v0.2.3 h1:ta4wFrO85+29H0papAbeMCavHrBuHDZ4bdKC1Zv8zlo=
Expand Down
2 changes: 1 addition & 1 deletion js/jsmodules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ import (
"go.k6.io/k6/js/modules/k6/data"
"go.k6.io/k6/js/modules/k6/encoding"
"go.k6.io/k6/js/modules/k6/execution"
"go.k6.io/k6/js/modules/k6/experimental/browser"
"go.k6.io/k6/js/modules/k6/experimental/tracing"
"go.k6.io/k6/js/modules/k6/grpc"
"go.k6.io/k6/js/modules/k6/html"
"go.k6.io/k6/js/modules/k6/http"
"go.k6.io/k6/js/modules/k6/metrics"
"go.k6.io/k6/js/modules/k6/ws"

"github.com/grafana/xk6-browser/browser"
expGrpc "github.com/grafana/xk6-grpc/grpc"
"github.com/grafana/xk6-redis/redis"
"github.com/grafana/xk6-timers/timers"
Expand Down
58 changes: 0 additions & 58 deletions js/modules/k6/experimental/browser/rootmodule.go
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion vendor/github.com/grafana/xk6-browser/api/browser.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 0 additions & 14 deletions vendor/github.com/grafana/xk6-browser/api/browser_type.go
View file
Open in desktop

This file was deleted.

8 changes: 7 additions & 1 deletion vendor/github.com/grafana/xk6-browser/api/frame.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading