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.

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

Refactor browser options #876

Merged
merged 22 commits into from May 18, 2023
Merged

Refactor browser options #876

merged 22 commits into from May 18, 2023

Conversation

ka3de
Copy link
Collaborator

@ka3de ka3de commented May 9, 2023

Description of changes

This PR modifies the list of supported browser options and how they are parsed based on #856 and #857. Specifically:

  • Removes support for browser options specified from JS/Goja layer when calling BrowserType.Launch and BrowserType.Connect methods. Instead some browser options will now be read from scenario browser options (see Add JS scenario options k6#3036 and Add GetScenarioOptions method to ExecutorConfig interface k6#3053) and others will be parsed from environment.
  • Changes browser options parsing to read every "environment related" option from environment variables. This affects options timeout, headless, debug, args, ignoreDefaultArgs and executablePath.
  • Removes support for slowMo browser option. This option will not be exposed by now until a decision is made about its "applicability" (browser vs browserContext) and its implementation (see Do not expose browser slowMo option #875).

Related: #854.
Closes #858, #874, #875.

Checklist

@ka3de ka3de self-assigned this May 9, 2023
@ka3de ka3de force-pushed the update/858-validate-browser-opts branch 7 times, most recently from a9039cb to d1e0029 Compare May 10, 2023 10:20
@ka3de
Copy link
Collaborator Author

ka3de commented May 10, 2023

A couple of comments about the implementation:

1. As suggested in #858, initially I tried to parse browser scenario options as soon as possible, so it could fail fast in case the setup is incorrect. Unfortunately, because these options are defined at the scenario level, they are only set at the VU context whenever an iteration starts. Therefore its parsing had to be done once we get called to one of the BrowserType exposed methods through Goja (e.g.: BrowserType.Launch and BrowserType.Connect), so similar as it was previously done.

2. The current implementation panics if the browser type option set is not chromium. It is ok for the test to fail, as this field has to be mandatory now, but it should not panic the way it does. This is because if the init process in launch and connect fails, including the options parsing, we panic from launch here and connect here. This is related also with previous issues we had related with non having panic "centralized" and we did some improvements to that in ???. I believe can be fixed by returning an error from both of these methods and then parsing it for possible "fatal" error from mapping layer and reacting accordingly. Another option is to use k6common.Throw instead of panic in the current launch and connect implementations. Because this PR is already quite long, my plan is to tackle this in a separate PR.

You can test this issue with the following script:

import { chromium } from 'k6/x/browser'

export const options = {
  scenarios: {
    ui: {
      executor: 'shared-iterations',
      options: {
        browser: {
            type: 'invalid',
        },
      },
    },
  },
};
  
export default async function() {
    const browser = chromium.launch();
    const context = browser.newContext();
    const page = context.newPage()
    
    try {
      await page.goto('https://test.k6.io/', { waitUntil: 'networkidle' })
    } finally {
      page.close();
      browser.close();
    }
}

@ka3de ka3de marked this pull request as ready for review May 10, 2023 10:47
@ka3de ka3de requested review from inancgumus and ankur22 May 10, 2023 14:36
@ka3de ka3de force-pushed the update/858-validate-browser-opts branch from d1e0029 to 8b65c58 Compare May 14, 2023 16:06
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

LGTM 👍

env/env.go Outdated Show resolved Hide resolved
@ka3de ka3de force-pushed the update/858-validate-browser-opts branch from 8b65c58 to 9d66d67 Compare May 15, 2023 12:53
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

Really great work here! 👏 Thanks for the well structured PR 🙂

I left a very minor nit, but happy for you to ignore it.

chromium/browser_type.go Outdated Show resolved Hide resolved
chromium/browser_type.go Show resolved Hide resolved
ka3de and others added 13 commits May 18, 2023 12:02
LaunchOptions struct serves also for browser options when using connect
method, therefore it makes sense to rename it to a more generic naming
that defines its purpose.

Co-authored-by: ankur22 <ankur.agarwal@grafana.com>
A new env package is a better fit for the current implementation of
IsRemoteBrowser than the k6ext package. In this new env package we can
define a lookupper function type to be used across the rest of the
codebase as a reference for environment variables lookup functions.
This will allow to have a common environment variable lookupper function
definition across the codebase.

Co-authored-by: İnanç Gümüş <inanc.gumus@grafana.com>
This function allows to retrieve the scenario options associated with
the given VU context. It is required in order to validate the browser
options defined at the scenario.options.browser level.
Change its type from []any to []string to match the actual browser
options struct. There is no need for it to be a slice of 'any'. Making
it a string slice will also ease the set up as environment variable
moving browser options to be parsed from ENV.
These methods are only used in the browser options parsing method,
therefore, by now, it makes more sense to keep them in the same file
instead than in a separate generic options implementation.
Avoid reading browser options defined at the JS layer for
BrowserType.Launch and BrowserType.Connect methods. These options will
be parsed from scenario options and environment variables.
This commit changes the way browser options are currently parsed in
order to read them browser options field defined at the scenario level
and also from environment variables.
The unit test should verify that any browser type set with a value
different than 'chromium', currently the only supported browser type,
returns an error. No browser type set also returns an error as this
field is now mandatory.
This allows to define a default env.Lookupper function at the
BrowserType level, which will be the one passed into browser options
parsing function, and that can be overwritten from tests (e.g.: test
browser) in order to be able to run tests that depend on environment
browser options in parallel.
ka3de added 9 commits May 18, 2023 12:03
Because browser type option is now a mandatory field to be able to run
k6 browser tests, we have to modify our mock VU implementation in order
to add this field. This is done through the definition of a default
scenario and a mock executor implementation for testing purposes.
Test browser had support for withContext option, which allowed to pass a
custom context to test browser, which would be the one set as VU
context. Usually this context was used in order to control from the test
the cancellation of the test browser.
The problem is that this context passed from the test implementation,
was "divergent" from the test runtime context set up when calling
setupHTTPTestModuleInstance() from test browser, which sets up all the
VU related mock data, including the mandatory browser type field inside
scenario options. Because this option is not mandatory, tests that were
using a custom context were failing due to scenario options not being
present in this context.

This has been fixed by not allowing custom contexts to be set for test
browser, and instead expose this component's context and context
cancelling function so they can be handled from tests implementation.

This commit also fixes the tests affected by this issue.
Based on #857, a decision
was made to not expose slowMo browser option by now, and try to move it
to the browser context level instead, which will provide more
flexibility.
Modify JS examples removing browser options that now should be set as
ENV variables and setting browser type option inside scenario options,
which is now a mandatory field for any browser test.
Also browser_args.js example is removed as it does not provide any value
beyond showing how to set a browser arg in the previous implementation;
and evaluate.js is modified in order to showcase 'evaluate' method
functionality without requiring the usage of 'ignoreBrowserArgs' option.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Work with VU.State.Options to validate browser options
3 participants