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

Merge browser in k6 #4056

Open
wants to merge 3,059 commits into
base: master
Choose a base branch
from
Open

Merge browser in k6 #4056

wants to merge 3,059 commits into from

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Nov 13, 2024

What?

Move xk6-browser inside k6 codebase

Why?

Browser is no longer experimental and it makes more sense to have a single repository

List of commands I've ran:

  1. git subtree add -P js/modules/k6/browser git@github.com:grafana/xk6-browser.git main - requires git-subtree - in practice can be done with a bunch of additional commands, just this is faser and easier
  2. cd js/modules/k6/browser/
  3. git rm -r CODE_OF_CONDUCT.md CODEOWNERS CONTRIBUTING.md docker-compose.yaml Dockerfile go.mod go.sum LICENSE Makefile README.md register.go SECURITY.md SUPPORT.md sync_register.go TROUBLESHOOTING.md release\ notes/ assets .golangci.yml .gitignore .github/
  4. git commit -m "browser: cleanup unnsecessary files" -n
  5. find . -iname '*.go' | xargs sed -i -r 's|github.com/grafana/xk6-browser|go.k6.io/k6/js/modules/k6/browser|g'
  6. edit js/jsmodules.go to import the new import
  7. go mod tidy && go mod vendor
  8. git commit -m "Move to using the in k6 browser code"

TODO:

  • general workflow on how this PR will be made
  • moving examples
  • running example tests
  • fix lint issues

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make 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)

inancgumus and others added 30 commits September 30, 2024 19:08
Bumps [go.k6.io/k6](https://github.com/grafana/k6) from 0.53.1-0.20240925100229-86ab6e3ceee8 to 0.54.0.
- [Release notes](https://github.com/grafana/k6/releases)
- [Commits](https://github.com/grafana/k6/commits/v0.54.0)

---
updated-dependencies:
- dependency-name: go.k6.io/k6
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
This fix ensures that the taskqueue is closed when the iteration ends.
If the taskqueue isn't closed when the iteration ends the k6 process
will hang indefinitely. We used to rely on page.close, but now that the
life cycle of the iteration is controlled by the VU we can safely rely
on the VU context to ensure that the taskqueue is closed (if it isn't
already closed).
`Description` should be `description`
This will allow us to write the test for all platforms that either work
with Control or Meta when performing keyboard actions. E.g.
Control+click on windows, and meta+click on mac to open a link in a new
window.
The test now doesn't test with combo keys which doesn't work yet.
Instead we're using dblClick.
The sobek code has been moved to the mapping layer, leaving the main
business logic sobek free for keyboard.press.
The sobek code has been moved to the mapping layer, leaving the main
business logic sobek free for keyboard.type.
This is to enable us to still work with the sync keyboard APIs with the
Sobek changes removed from the business logic side of keyboard.
waitForSelector may result in a confusing error message:

        selector did not resolve to any element

We're now explaining which selector didn't resolve to an element.
page.on only worked with console message, but we will need it to work
for metric messages too. This change allows us to send any type and
perform the type check in the handler.
It will need to be used in the page mapping.
Using switch to allow for multiple events.
inancgumus and others added 21 commits November 7, 2024 09:44
NewBrowserContextOptions no longer makes sense.
DefaultBrowserContextOptions also reads better when we look at the code.
We can easily tell we're working with the default options, not just a
new BrowserContextOptions value.
The main type here should BrowserContextOptions.
Bumps [go.k6.io/k6](https://github.com/grafana/k6) from 0.54.0 to 0.55.0.
- [Release notes](https://github.com/grafana/k6/releases)
- [Commits](v0.54.0...v0.55.0)

---
updated-dependencies:
- dependency-name: go.k6.io/k6
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
The fix requires the reuse of waitFor, but it also requires the handle
value to be returned. This refactors waitFor to return Handle.
The fix is in waitFor, and waitForSelector just needs to call waitFor
to enable the fix.

It basically will auto retry up to 20 times when certain errors are
received from chrome. This can happens when the underlying DOM is
changing due to a navigation, and the expected element that matches the
selector is in the newly navigated DOM.
…152804da63b85'

git-subtree-dir: js/modules/k6/browser
git-subtree-mainline: 0411e28
git-subtree-split: 1974b8a
@mstoykov mstoykov requested a review from a team as a code owner November 13, 2024 15:24
@mstoykov mstoykov requested review from inancgumus and ankur22 and removed request for a team November 13, 2024 15:24
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 👍 What about the rest of the TODO list? Tests are also failing on Windows 🤔

@mstoykov
Copy link
Contributor Author

the windows part is grafana/xk6-browser#571 ;)

@inancgumus
Copy link
Member

For the heads-up, there was one more update after this PR:

grafana/xk6-browser#1536

@mstoykov
Copy link
Contributor Author

I left the steps as I expect I/we are going to redo this PR once we fix even more stuff in browser. For example the lint errors seem like a good thing to be fixed before we merge it, and it will be way nicer to fix them in the browser repo.

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.

4 participants