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

fix: use yarn instead of npx with yarn.lock #914

Merged
merged 3 commits into from Jun 13, 2023

Conversation

MikeMcC399
Copy link
Collaborator

@MikeMcC399 MikeMcC399 commented May 16, 2023

This PR resolves an issue calling commands cypress cache list and cypress cache verify if the project is using Yarn Plug'n'Play (introduced through PR #599 from @PilotConway). In this case the commands were not finding the installed version of Cypress.

Changes

The following commands, which are called internally by the action, are replaced if the action is working from a yarn.lock file. This is the case for Yarn Classic and Yarn Modern projects. Although the issue only affects Yarn Modern using Yarn Plug'n'Play configuration, the change is activated for all types of Yarn projects.

The following commands are retained for npm and pnpm projects:

npx cypress cache list
npx cypress verify

For Yarn projects they are changed to use instead:

yarn cypress cache list
yarn cypress verify

Issue

When .github/workflows/example-yarn-modern-pnp.yml is run, it shows npx being called and not finding the installed version of cypress.

For example job 4959218510 shows:

/usr/local/bin/npx cypress cache list
npm WARN exec The following package was not found and will be installed: cypress@12.12.0
┌─────────┬───────────────────┐
│ version │ last used         │
├─────────┼───────────────────┤
│ 12.11.0 │ a few seconds ago │
├─────────┼───────────────────┤
│ 12.12.0 │ 3 days ago        │
└─────────┴───────────────────┘
/usr/local/bin/npx cypress verify

With Yarn Plug'n'Play there is no directory node_modules set up and therefore npx cannot find cypress and it installs instead the latest version, which may or may not correspond to the version specified in yarn.lock.

Verification

Check the workflow results:

  1. All workflows should be successful.
  2. example-basic.yml and example-basic-pnpm.yml should use npx when calling cypress cache list or cypress verify.
  3. Each of the Yarn workflows should use yarn when calling cypress cache list and cypress verify. Only one version of Cypress should be shown by cypress cache list.

@MikeMcC399
Copy link
Collaborator Author

Job 8937982597 shows successful selection of yarn cypress cache list and yarn cypress verify from example-yarn-modern-pnp.yml. Only one version of Cypress is show in the cache list, which is correct.

/usr/local/bin/yarn cypress cache list
┌─────────┬───────────────────┐
│ version │ last used         │
├─────────┼───────────────────┤
│ 12.11.0 │ a few seconds ago │
└─────────┴───────────────────┘
/usr/local/bin/yarn cypress verify

[STARTED] Task without title.
[SUCCESS] Task without title.

@MikeMcC399 MikeMcC399 marked this pull request as ready for review May 16, 2023 11:58
@PilotConway
Copy link
Contributor

Question: Any plans to use this same approach on the cypress test run itself? That seems to be the only other place hardcoded to npx, and fixing that line would remove the requirement for my command fix completely.

https://github.com/MikeMcC399/github-action/blob/2a93c9f278747ca1ce07982dbafb7f50f6823777/index.js#L634-L647

I did run this on our internal code to test and it was able to find and use my installed cypress version so this PR is working great for us! Thanks!

@MikeMcC399
Copy link
Collaborator Author

@PilotConway

Thanks very much for cross-checking this PR on your own repo. That is very valuable!

@PilotConway
Copy link
Contributor

@MikeMcC399 Awesome, sounds good! Thanks for putting in fixes for this stuff, I know my team definitely appreciates it. 😄

@MikeMcC399 MikeMcC399 force-pushed the yarn-no-npx branch 3 times, most recently from 9f6f47c to ade1551 Compare May 22, 2023 19:31
@MikeMcC399 MikeMcC399 force-pushed the yarn-no-npx branch 5 times, most recently from 19a63af to 8af941e Compare May 30, 2023 16:17
@MikeMcC399
Copy link
Collaborator Author

@nagash77
This PR from mid May could also benefit from some attention. So far it has only been assigned, then re-assigned.

@MikeMcC399 MikeMcC399 force-pushed the yarn-no-npx branch 5 times, most recently from a3cd69d to 8e7ff55 Compare June 7, 2023 15:00
@MikeMcC399
Copy link
Collaborator Author

Hi @warrensplayer

Is there any chance you could review this PR sometime soon?

@warrensplayer
Copy link
Contributor

@MikeMcC399 Looking at this now. Sorry for the delay.

Copy link
Contributor

@warrensplayer warrensplayer left a comment

Choose a reason for hiding this comment

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

Two small requests

dist/index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@warrensplayer warrensplayer requested review from a team June 12, 2023 21:25
Co-Authored-By: Stokes Player <1702361+warrensplayer@users.noreply.github.com>
@MikeMcC399
Copy link
Collaborator Author

@warrensplayer

Looking at this now. Sorry for the delay.

Thank you for your review! I included your suggestion for code optimization. All tests are passing, including the specific verification tests I listed in the original post.

@warrensplayer warrensplayer merged commit e08da49 into cypress-io:master Jun 13, 2023
115 checks passed
@github-actions
Copy link

🎉 This PR is included in version 5.8.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MikeMcC399 MikeMcC399 deleted the yarn-no-npx branch June 14, 2023 04:46
MikeMcC399 added a commit to MikeMcC399/github-action that referenced this pull request Jun 15, 2023
nagash77 pushed a commit that referenced this pull request Jun 15, 2023
MikeMcC399 added a commit to MikeMcC399/github-action that referenced this pull request Jul 16, 2023
Co-authored-by: Stokes Player <1702361+warrensplayer@users.noreply.github.com>
MikeMcC399 added a commit to MikeMcC399/github-action that referenced this pull request Jul 16, 2023
Co-authored-by: Stokes Player <1702361+warrensplayer@users.noreply.github.com>
@MikeMcC399
Copy link
Collaborator Author

MikeMcC399 commented Jul 18, 2023

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

6 participants