Limit browser implementation to hold a single browserContext #929

ankur22 · 2023-06-12T15:29:38Z

Description of changes

This PR is one of a few that will be used to help enforce a single browserContext per browser per iteration. The reason for this change can be found in this directly linked issue and this overarching issue. The summary is that we want to be able to predict the resource requirements of how many browsers a test run may need, and the first step towards this goal is to be able to predict the number of browserContexts a test run will need.

At the moment (in this PR) we enforce a single browserContext per iteration, which also means we allow the user to close() the existing browserContext and create a new one with NewContext. Any objects still associated to the previous browserContext will be closed and the user will not be able to work with those objects. In a later PR we will change this behaviour so once a browserContext is setup, no other browserContext can be created, even if the existing one is closed.

export default async function() {
  const context = browser.newContext();
  const page = context.newPage();

  // const context2 = context.newContext(); // If uncommented, it would cause an error.

  context.close(); // page cannot be used after this call

  // await page.goto("https://test.k6.io"); // If uncommented, it would cause an error.

  const context2 = context.newContext();

  // await page.goto("https://test.k6.io"); // If uncommented, it would cause an error.

  const page2 = context2.newPage();
  await page2.goto("https://test.k6.io");
  ...

Checklist

Tasks

Give feedback

Written tests for the changes
Update k6 documentation (if relevant)
Generate TS definitions (if relevant)
Options

To enable us to predicate and have a better idea of how much resource a test run will require, we want to restrict the number of browser- Contexts per vu to be one. This will ensure that a single test scenario which requires 1 vu and 50 iterations could work with a single browser and a new browserContext per iteration. Users will need to close the existing browserContext before creating a new one. We feel that this restriction along with scenarios should still work for majority of our users.

This check when NewContext is called on browser will ensure that we only ever have one browserContext at any one point. If a user wishes to create a new browserContext, they will need to close the existing one first.

ankur22 · 2023-06-12T15:40:43Z

common/browser.go

+	browserCtx := b.context
+	b.contextMu.RUnlock()
+	if browserCtx != nil {
+		return nil, errors.New("close the existing browser context before creating a new one")


Let me know how you feel about this. I could change this in this PR (was going to do it in a later PR) so that we don't allow more than one browserContext to be setup as we originally wanted.

ankur22 · 2023-06-12T15:53:45Z

common/browser.go

@@ -50,8 +50,7 @@ type Browser struct {
 	// A *Connection is saved to this field, see: connect().
 	conn connection

-	contextsMu     sync.RWMutex


I've done a stress test on this mutex removal change, and it all seems to work still.

😱🤞🙃

Since all interactions with the browserContext in the browser are synchronous and no async actions occur on browserContext that could cause it to change between two or more goroutines - browser.context is set when browser.NewContext is called, and unset when browser.close is called.

ankur22 · 2023-06-12T15:59:27Z

I haven't added a docs issue to this PR, as there are still two more PRs to create (as directed by the original issue). Once the final PR is done I will work on the docs.

ka3de

Overall LGTM 👍 Just a couple of small comments.

common/browser.go

inancgumus

Looks good! Minor skippable suggestions suggestions.

common/browser.go

tests/keyboard_test.go

When multiple k6 instances connect to a single chrome instance, the page that is created by each k6 instance is also recreated in all k6 instances that are connected to the same browser. We don't do the same for browserContexts. When a page is recreated in one k6 instance for another k6 instance, the browserContext will not be there, so instead for now we should still work with the defaultContext.

Co-authored-by: ka3de <daniel.jimenez@grafana.com>

Co-authored-by: İnanç Gümüş <inanc.gumus@grafana.com>

ka3de

LGTM.

inancgumus

LGTM 👍

ankur22 added 2 commits June 12, 2023 16:04

Add check to ensure only one browserContext

cad4a92

This check when NewContext is called on browser will ensure that we only ever have one browserContext at any one point. If a user wishes to create a new browserContext, they will need to close the existing one first.

ankur22 requested review from inancgumus and ka3de June 12, 2023 15:29

ankur22 mentioned this pull request Jun 12, 2023

Enforce 1 BrowserContext per iteration #925

Closed

ankur22 changed the title ~~Enforce 1 browser context~~ Limit browser implementation to hold a single browserContext Jun 12, 2023

ankur22 commented Jun 12, 2023

View reviewed changes

ankur22 force-pushed the refactor/925-enfore-1-browserContext branch from b743c16 to 177cf7e Compare June 12, 2023 15:56

ankur22 marked this pull request as draft June 12, 2023 16:06

ka3de reviewed Jun 13, 2023

View reviewed changes

common/browser.go Outdated Show resolved Hide resolved

common/browser.go Outdated Show resolved Hide resolved

ankur22 mentioned this pull request Jun 13, 2023

TestMultiConnectToSingleBrowser is flaky #861

Closed

inancgumus reviewed Jun 13, 2023

View reviewed changes

common/browser.go Outdated Show resolved Hide resolved

common/browser.go Outdated Show resolved Hide resolved

tests/keyboard_test.go Show resolved Hide resolved

ankur22 and others added 3 commits June 13, 2023 15:32

Apply suggestions from code review

626d4be

Co-authored-by: ka3de <daniel.jimenez@grafana.com>

Update error to include context details

0a37f5b

Co-authored-by: İnanç Gümüş <inanc.gumus@grafana.com>

ankur22 force-pushed the refactor/925-enfore-1-browserContext branch from e413f52 to 0a37f5b Compare June 13, 2023 14:45

ankur22 marked this pull request as ready for review June 13, 2023 14:45

ankur22 mentioned this pull request Jun 13, 2023

Investigate whether it's possible to skip page creation when browserContext is not found #933

Open

ka3de approved these changes Jun 13, 2023

View reviewed changes

ankur22 requested a review from inancgumus June 13, 2023 15:26

inancgumus approved these changes Jun 13, 2023

View reviewed changes

ankur22 merged commit b8b1b5f into main Jun 14, 2023

ankur22 deleted the refactor/925-enfore-1-browserContext branch June 14, 2023 08:27

ankur22 mentioned this pull request Jun 30, 2023

Update context, newContext and newPage description for browser module grafana/k6-docs#1242

Merged

ka3de mentioned this pull request Jul 19, 2023

Docs: Migration guide for k6 browser v1.0.0 API grafana/k6-docs#1259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit browser implementation to hold a single browserContext #929

Limit browser implementation to hold a single browserContext #929

ankur22 commented Jun 12, 2023 •

edited

Loading

Tasks

ankur22 Jun 12, 2023 •

edited

Loading

ankur22 Jun 12, 2023 •

edited

Loading

inancgumus Jun 13, 2023

ankur22 commented Jun 12, 2023

ka3de left a comment

inancgumus left a comment •

edited

Loading

ka3de left a comment

inancgumus left a comment

Limit browser implementation to hold a single browserContext #929

Limit browser implementation to hold a single browserContext #929

Conversation

ankur22 commented Jun 12, 2023 • edited Loading

Description of changes

Checklist

Tasks

ankur22 Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

ankur22 Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

inancgumus Jun 13, 2023

Choose a reason for hiding this comment

ankur22 commented Jun 12, 2023

ka3de left a comment

Choose a reason for hiding this comment

inancgumus left a comment • edited Loading

Choose a reason for hiding this comment

ka3de left a comment

Choose a reason for hiding this comment

inancgumus left a comment

Choose a reason for hiding this comment

ankur22 commented Jun 12, 2023 •

edited

Loading

ankur22 Jun 12, 2023 •

edited

Loading

ankur22 Jun 12, 2023 •

edited

Loading

inancgumus left a comment •

edited

Loading