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: fix corepack wiring in integration tests #10044

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jenseng
Copy link
Contributor

@jenseng jenseng commented Feb 25, 2025

Description

Spun out from #10023 (comment)

setup_package_manager.sh exports PATH, so we need to source it for the isolated corepack shims to actually get used in tests. Otherwise certain npm integration tests can fail, depending on the global npm version and whether corepack's npm shims have been explicitly enabled globally. This can lead to false positives and wasted time debugging integration tests locally.

This hasn't been an issue under GitHub Actions, since actions/setup-node is installing the same version of npm (10.5.0) that the tests expect, but could otherwise become problematic down the road.

Fixing this uncovers two new problems around testing under Windows, which this PR also addresses:

  • the corepack path needs to be POSIX-ified, since C:\Users\RUNNER~1\... contains backslashes and the name separator (:)
  • corepack uses PATHEXT (via cmd-extension) to determine the casing of its shims' file extension (e.g. npm.cmd vs npm.CMD), whereas node's bundled npm.cmd is always lowercase ... while we could update all the tests to accept both styles, the easiest thing to do is just to downcase PATHEXT.

Testing Instructions

Integration tests 🥳

setup_package_manager.sh exports `PATH`, so we need to source it for the
isolated corepack to actually get enabled. Otherwise certain npm integration
tests can fail, depending on the global npm version and whether corepack's
npm shims have been explicitly enabled globally. This hasn't been an issue
under GitHub Actions, since actions/setup-node is installing the same version
of npm (10.5.0) that the tests expect, but could otherwise become problematic
down the road.

Fixing this uncovers two new problems around testing under Windows, which this
PR also addresses:

 - the corepack path needs to be POSIX-ified, since `C:\Users\RUNNER~1\...`
   contains backslashes and the name separator (:)
 - corepack uses `PATHEXT` to determine the casing of its shims (e.g. npm.cmd vs
   npm.CMD), whereas node always uses lowercase ... while we could update all
   the tests to accept both styles, the easiest thing to do is just to downcase
   `PATHEXT`.
@jenseng jenseng requested a review from a team as a code owner February 25, 2025 23:51
@turbo-orchestrator turbo-orchestrator bot added the needs: triage New issues get this label. Remove it after triage label Feb 25, 2025
Copy link

vercel bot commented Feb 25, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-basic-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 11:51pm
examples-designsystem-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 11:51pm
examples-native-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 11:51pm
examples-svelte-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 11:51pm
examples-tailwind-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 11:51pm
examples-vite-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 11:51pm

Copy link

vercel bot commented Feb 25, 2025

@jenseng is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: triage New issues get this label. Remove it after triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant