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

Remove K6_BROWSER_ENABLED flag requirement #3221

Closed
wants to merge 1 commit into from

Conversation

ka3de
Copy link
Contributor

@ka3de ka3de commented Jul 21, 2023

What?

Removes the requirement for the K6_BROWSER_ENABLED environment variable in order to execute tests that use the experimental browser module.

Why?

Currently (as of grafana/xk6-browser@a8ebe8c) 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

@github-actions github-actions bot requested review from imiric and mstoykov July 21, 2023 07:39
@ka3de ka3de requested a review from ankur22 July 21, 2023 07:40
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2023

Codecov Report

Merging #3221 (812d0ed) into master (703970e) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 812d0ed differs from pull request most recent head 162b827. Consider uploading reports for the commit 162b827 to get more accurate results

@@            Coverage Diff             @@
##           master    #3221      +/-   ##
==========================================
- Coverage   72.68%   72.67%   -0.02%     
==========================================
  Files         259      258       -1     
  Lines       19864    19849      -15     
==========================================
- Hits        14439    14425      -14     
  Misses       4521     4521              
+ Partials      904      903       -1     
Flag Coverage Δ
ubuntu 72.61% <100.00%> (-0.02%) ⬇️
windows 72.51% <100.00%> (-0.02%) ⬇️

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

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

... and 3 files with indirect coverage changes

Copy link
Contributor

@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.

Thanks for this 👍

I think we can remove the whole of the experimental/browser/rootmodule.go directory and file. We should be able to directly work with the root module that we define in the browser module -- replace "github.com/grafana/xk6-browser/browser" in jsmodules.go.

We created the RootModule wrapper in the experimental/browser pkg in k6 to enable us to work with the env var.

@ka3de
Copy link
Contributor Author

ka3de commented Jul 21, 2023

We should be able to directly work with the root module that we define in the browser module -- replace "github.com/grafana/xk6-browser/browser" in jsmodules.go.

Right, thanks for the heads up, I think that makes sense, as I see there are other examples of using the external implementation directly. Let me address this.

Currently (as of grafana/xk6-browser@a8ebe8c) 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.
@ka3de ka3de force-pushed the 3197-remove-browser-enabled-flag branch from 15cb14f to 162b827 Compare July 21, 2023 08:52
@mstoykov
Copy link
Contributor

Shouldn't this be merged after we update xk6-browser in k6?

@ka3de
Copy link
Contributor Author

ka3de commented Jul 24, 2023

Shouldn't this be merged after we update xk6-browser in k6?

Right, I was assuming we will upgrade the current xk6-browser version, which release is planned for this Friday, at some point before the next k6 release. But yes, in practice we can wait for this to happen before merging this PR.

@ka3de
Copy link
Contributor Author

ka3de commented Jul 28, 2023

Closing as superseded by #3235.

@ka3de ka3de closed this Jul 28, 2023
@ka3de ka3de deleted the 3197-remove-browser-enabled-flag branch November 8, 2023 08:44
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.

K6_BROWSER_ENABLED flag is redundant with browser type scenario option
4 participants