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

chore: update dependency @elastic/synthetics and Playwright #285

Merged
merged 12 commits into from
Aug 10, 2022

Conversation

kyungeunni
Copy link
Contributor

@kyungeunni kyungeunni commented Jul 26, 2022

closes #262 and #263
relates to elastic/playwright#1

Summary

This PR updates a couple of dependencies, the latest @elastic/synthetics agent (0.0.1-beta.31) and playwright version that the agent is using.
This update is especially exciting as Synthetics Recorder now generates scripts with Locator API 🎉

Some notable changes:

  • Uses Node version to 16.15.0, npm >= 7 (package-lock.json is now version 2)
  • Removes npm-force-resolutions preinstall hook as it pollutes the npm install behaviour(since npm@7)
  • Adds temporary type definition around ActionInContext and Step to make it compatible with the newer Playwright. These type definitions will be deleted once synthetics agent exports the compatible type definition again
  • After updating Node version, we needed to build a new Docker image. With the new image and the new node, CI was having some issues with the permission when executing npm scripts. It seems like the docker tries to run the node executable installed by the host workspace that is mounted as volume. As a workaround, we install node dependencies rather than mounting the workspace volume when building the image.

How to validate this change

Pull this branch and run npm ci and run npm run dev. Confirm that everything works as before, also note that the generated code now contains page.locator(...).

TODO

  • Find out what has changed to the signal url when recording the script. It's making the test fail as it waits for the navigation to the URL with the specific querystring which usually generated randomly on each navigation.
    edit: _The same behaviour is observed in the main branch as well so thus it is not related to the playwright version update. will address it separately

Next Steps

  • Test releases
  • Delegate the code generation to Synthetics agent

@apmmachine
Copy link
Contributor

apmmachine commented Jul 26, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-08-09T18:02:12.162+0000

  • Duration: 19 min 54 sec

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /test all : Re-trigger the build for all the stages.

@kyungeunni kyungeunni force-pushed the 262-update-playwright branch 2 times, most recently from 26762d3 to 86fff83 Compare July 26, 2022 20:25
@kyungeunni kyungeunni force-pushed the 262-update-playwright branch from 86fff83 to c2ea3de Compare July 27, 2022 13:26
@kyungeunni
Copy link
Contributor Author

/test

@kyungeunni
Copy link
Contributor Author

/test all

@kyungeunni kyungeunni force-pushed the 262-update-playwright branch 7 times, most recently from ec8a1fa to d76501f Compare July 29, 2022 17:09
@kyungeunni kyungeunni force-pushed the 262-update-playwright branch from 5c91e17 to 9b58314 Compare August 4, 2022 17:12
@kyungeunni kyungeunni marked this pull request as ready for review August 8, 2022 09:43
@kyungeunni kyungeunni self-assigned this Aug 8, 2022
Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Did a first pass of the code, Looks great 🎉

I will try to run and test locally.

package.json Outdated Show resolved Hide resolved
scripts/install-pw.js Show resolved Hide resolved
README.md Outdated
await page2.click(':nth-match(a:has-text("babel-minify"), 3)');
await page2.close();
});
2. Confirm the Playwright version from Synthetics agent's `package.json`, we will refer it as `v${NEW_VERSION}`. Checkout to `NEW_VERSION` tag, and create a new branch from there:
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing and assumes Synthetics agent PW version with the branch version which is not we are doing with the rebasing. Instead it would be the latest version of Microsoft playwright that was released.

I do understand the intention here that we can keep a versioning scheme, But then we would have to assume the synthetics agent is always updated to the PW latest version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might need to sync on this, but maybe we have a different approach on this.

Are you saying that the recorder should use the latest PW (let's say 1.24 or even higher that is not released) whereas the synthetics agent uses an older version(1.20.1 as of today)? I thought the intention was to use the same version of the playwright on both sides so we don't have any surprises.

For this PR what I did was:

  1. fetch main from upstream (PW latest version is 1.25.0-next)
  2. create a dev branch that is sync up with the main and an extra commit with the modification for the recorder
  3. Synthetics agent is using 1.20.1, so I checkout to v1.20.1, create a new branch named 1.20.1-recorder, and then cherry-pick the commit from the dev branch
  4. Use 1.20.1-recorder branch from the recorder

But from my understanding, you are suggesting we do:

  1. fetch main from upstream (PW latest version is 1.25.0-next)
  2. create a dev branch that is sync up with the main and an extra commit with the modification for the recorder
  3. Use dev branch from the recorder

Copy link
Member

Choose a reason for hiding this comment

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

Myself and Kyungeun Synced on this separately, It was a misunderstanding in my part. The outcome is to update the docs with bit more detail so we can give more clarity around what versioning scheme we want to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the doc, let me know what you think! 🙏

@kyungeunni kyungeunni merged commit f1c6351 into elastic:main Aug 10, 2022
@kyungeunni kyungeunni deleted the 262-update-playwright branch August 10, 2022 11:07
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.

Migrate Elastic Playwright fork to later version of official Playwright
3 participants