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

test: use installed cypress version in yarn pnp example #903

Merged
merged 9 commits into from Jun 1, 2023

Conversation

MikeMcC399
Copy link
Collaborator

@MikeMcC399 MikeMcC399 commented May 8, 2023

This PR removes the use of yarn dlx from

and changes running Cypress from

command: yarn dlx cypress run to

command: yarn cypress run.

Issue

yarn dlx cypress run causes the latest version of Cypress to be used, with no regard of the version coming from package.json. This should not be the default way of demonstrating github-action with Yarn Plug'n'Play. The Cypress test should run using the exact version installed by yarn install.

The problem can be seen in job 8771406837 from PR #904 which reverts the example to installing Cypress 12.9.0. Despite this, the example actually runs with Cypress 12.11.0.

Verification

Install an older version of Cypress e.g. with yarn install cypress@12.10.0 -D -E, run the workflow .github/workflows/example-yarn-modern-pnp.yml and check that the Cypress test runs with the older, selected version.

@MikeMcC399
Copy link
Collaborator Author

A successful run is recorded in job 8771239739.

@MikeMcC399 MikeMcC399 marked this pull request as ready for review May 8, 2023 07:12
@PilotConway
Copy link
Contributor

@MikeMcC399 I should have looked more into npx, I thought it was acting the same as yarn dlx and installing a new package every time. So by using yarn dlx in my PR I assumed I would actually keep it consistent with how the action worked by default, but now I'm seeing that isn't the case. 🤦‍♂️

Good catch!

@MikeMcC399
Copy link
Collaborator Author

@PilotConway

Many thanks for your confirmation! I'm still ramping up on Yarn Plug'n'Play so I didn't catch this issue until after your PR was merged.

npx will only install a package if one isn't already available. It will use the version defined in local dependencies if possible.

@MikeMcC399 MikeMcC399 force-pushed the yarn-pnp-no-dlx branch 4 times, most recently from 4d6fd9b to a72943d Compare May 12, 2023 13:55
@MikeMcC399
Copy link
Collaborator Author

MikeMcC399 commented May 25, 2023

@marktnoonan

Could we get this merged?

It would avoid Yarn Plug'n'Play users copying an incorrect example from 73fc04a which might trip them up when new versions of Cypress are released and they have chosen to remain with an earlier version. Without this PR the action will always run with the latest version of Cypress when Yarn Modern (version 2 and later) is configured with the default setting of pnp.

After the PR is merged, the action uses the version of Cypress taken from package.json => yarn.lock and does not force the use of cypress@latest.

@MikeMcC399
Copy link
Collaborator Author

Hi @nagash77
Can this PR be moved along? It was approved 2 weeks ago by @AtofStryker. A second approval is missing.

@AtofStryker AtofStryker merged commit 21887a6 into cypress-io:master Jun 1, 2023
117 checks passed
@MikeMcC399 MikeMcC399 deleted the yarn-pnp-no-dlx branch June 1, 2023 17:20
@github-actions
Copy link

github-actions bot commented Jun 1, 2023

🎉 This PR is included in version 5.8.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants