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: Ensure that C3 e2e tests are running against local wrangler #7809

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

Conversation

CarmenPopoviciu
Copy link
Contributor

@CarmenPopoviciu CarmenPopoviciu commented Jan 17, 2025

Ensure that C3 e2e tests are running against wrangler@beta in CI

This is one smol incremental step towards fixing #7800.


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: internal change

Copy link

changeset-bot bot commented Jan 17, 2025

⚠️ No Changeset found

Latest commit: fc60f0b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@CarmenPopoviciu CarmenPopoviciu added the skip-pr-description-validation Skip validation of the required PR description format label Jan 17, 2025
Copy link
Contributor

github-actions bot commented Jan 17, 2025

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-wrangler-7809

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7809/npm-package-wrangler-7809

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-wrangler-7809 dev path/to/script.js
Additional artifacts:

cloudflare-workers-bindings-extension:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-cloudflare-workers-bindings-extension-7809 -O ./cloudflare-workers-bindings-extension.0.0.0-v15a0aa160.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v15a0aa160.vsix

create-cloudflare:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-create-cloudflare-7809 --no-auto-update

@cloudflare/kv-asset-handler:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-cloudflare-kv-asset-handler-7809

miniflare:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-miniflare-7809

@cloudflare/pages-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-cloudflare-pages-shared-7809

@cloudflare/unenv-preset:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-cloudflare-unenv-preset-7809

@cloudflare/vite-plugin:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-cloudflare-vite-plugin-7809

@cloudflare/vitest-pool-workers:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-cloudflare-vitest-pool-workers-7809

@cloudflare/workers-editor-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-cloudflare-workers-editor-shared-7809

@cloudflare/workers-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-cloudflare-workers-shared-7809

@cloudflare/workflows-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12954558317/npm-package-cloudflare-workflows-shared-7809

Note that these links will no longer work once the GitHub Actions artifact expires.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241230.2
workerd 1.20250124.0 1.20250124.0
workerd --version 1.20250124.0 2025-01-24

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/wrangler-beta-ci branch 3 times, most recently from 9ed291c to 3059047 Compare January 17, 2025 14:36
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/wrangler-beta-ci branch from c17b408 to 7624883 Compare January 22, 2025 14:24
@CarmenPopoviciu CarmenPopoviciu marked this pull request as ready for review January 22, 2025 14:25
@CarmenPopoviciu CarmenPopoviciu requested review from a team as code owners January 22, 2025 14:25
@CarmenPopoviciu CarmenPopoviciu added the e2e Run e2e tests on a PR label Jan 22, 2025
@CarmenPopoviciu CarmenPopoviciu changed the title wip: use wrangler@beta for C3 templates in CI chore: Ensure that C3 e2e tests are running against local wrangler Jan 22, 2025
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/wrangler-beta-ci branch 3 times, most recently from c2181e6 to 01799a3 Compare January 22, 2025 18:18
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/wrangler-beta-ci branch from 01799a3 to 573796e Compare January 23, 2025 09:10
@@ -88,9 +89,14 @@ export async function getLatestPackageVersion(packageSpecifier: string) {
*/
export const installWrangler = async () => {
const { npm } = detectPackageManager();
const wrangler = process.env.CI
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make this CI only?

What if you make the WRANGLER env var more specific (e.g. C3_WRANGLER_OVERRIDE or something) and then use that as the flag to override the wrangler@latest?

Then we can use this for local testing too.


export function packWrangler(): string {
const pathToWrangler = path.resolve(__dirname, "../../wrangler");
execSync("pnpm pack --pack-destination ./pack", { cwd: pathToWrangler });
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this doesn't quite do what we need.
This will pack Wrangler, yes, but it will change the local Wrangler dependencies to published ones. For example,

"miniflare": "workspace:*",

becomes

"miniflare": "3.20241230.2",

which means that when C3 installs this version of Wrangler it will pull in the published miniflare, instead of the locally built one.

This is notably problematic because one of the reasons we wanted to do this was that the transitive dependency on @cloudflare/unenv-preset that was added did not cause a failure in the C3 tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could leverage turbo to do that more automatically?

@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/wrangler-beta-ci branch from 859220b to 1a3f773 Compare January 23, 2025 12:12
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

This is good to get us unblocked for releases.
Thanks!

@petebacondarwin
Copy link
Contributor

Uurgh!

npm error ERESOLVE could not resolve
npm error
npm error While resolving: @remix-run/[email protected]
npm error Found: [email protected]
npm error node_modules/wrangler
npm error   dev wrangler@"^0.0.0-d7210ecd3" from the root project
npm error
npm error Could not resolve dependency:
npm error peerOptional wrangler@"^3.28.2" from @remix-run/[email protected]
npm error node_modules/@remix-run/dev
npm error   dev @remix-run/dev@"^2.15.2" from the root project
npm error
npm error Conflicting peer dependency: [email protected]
npm error node_modules/wrangler
npm error   peerOptional wrangler@"^3.28.2" from @remix-run/[email protected]
npm error   node_modules/@remix-run/dev
npm error     dev @remix-run/dev@"^2.15.2" from the root project
npm error
npm error Fix the upstream dependency conflict, or retry
npm error this command with --force or --legacy-peer-deps
npm error to accept an incorrect (and potentially broken) dependency resolution.

@@ -88,9 +88,10 @@ export async function getLatestPackageVersion(packageSpecifier: string) {
*/
export const installWrangler = async () => {
const { npm } = detectPackageManager();
const wrangler = process.env.CI ? `wrangler@beta` : "wrangler@latest";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe try:

Suggested change
const wrangler = process.env.CI ? `wrangler@beta` : "wrangler@latest";
const wrangler = process.env.CI ? `wrangler@beta --force` : "wrangler@latest";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm error Invalid tag name "beta --force" of package "wrangler@beta --force": Tags may not have any characters that encodeURIComponent encodes.

😢 unfortunately this breaks tests for all package managers

@CarmenPopoviciu CarmenPopoviciu removed the e2e Run e2e tests on a PR label Jan 23, 2025
@CarmenPopoviciu
Copy link
Contributor Author

Uurgh!

npm error ERESOLVE could not resolve
npm error
npm error While resolving: @remix-run/[email protected]
npm error Found: [email protected]
npm error node_modules/wrangler
npm error   dev wrangler@"^0.0.0-d7210ecd3" from the root project
npm error
npm error Could not resolve dependency:
npm error peerOptional wrangler@"^3.28.2" from @remix-run/[email protected]
npm error node_modules/@remix-run/dev
npm error   dev @remix-run/dev@"^2.15.2" from the root project
npm error
npm error Conflicting peer dependency: [email protected]
npm error node_modules/wrangler
npm error   peerOptional wrangler@"^3.28.2" from @remix-run/[email protected]
npm error   node_modules/@remix-run/dev
npm error     dev @remix-run/dev@"^2.15.2" from the root project
npm error
npm error Fix the upstream dependency conflict, or retry
npm error this command with --force or --legacy-peer-deps
npm error to accept an incorrect (and potentially broken) dependency resolution.

I like it tho how this is only a problem in npm 🙃 🥲

@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/wrangler-beta-ci branch 3 times, most recently from 39d3350 to 5450688 Compare January 23, 2025 15:55
@lrapoport-cf lrapoport-cf added caretaking Priority for caretaking maintenance Maintenance task labels Jan 23, 2025
@penalosa penalosa force-pushed the carmen/wrangler-beta-ci branch from 5450688 to fc60f0b Compare January 24, 2025 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
caretaking Priority for caretaking maintenance Maintenance task skip-pr-description-validation Skip validation of the required PR description format
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

3 participants