-
Notifications
You must be signed in to change notification settings - Fork 925
Skip remote E2E tests when no auth available #9826
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
Conversation
|
afc5e63
to
547637d
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
WRANGLER: node --no-warnings ${{ github.workspace}}/packages/wrangler/bin/wrangler.js | ||
WRANGLER_IMPORT: ${{ github.workspace}}/packages/wrangler/wrangler-dist/cli.js | ||
MINIFLARE_IMPORT: ${{ github.workspace}}/packages/miniflare/dist/src/index.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See tools/e2e/runIndividualE2EFiles.ts
for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits but nice QOL improvement
|
||
// If the user hasn't specifically set an inspector port, set it to 0 to reduce port conflicts | ||
const inspectorPort = | ||
command.includes(`--inspector-port`) || !command.startsWith("wrangler dev") | ||
? "" | ||
: " --inspector-port 0"; | ||
|
||
// If the user hasn't specifically set a Worker port, set it to 0 to reduce port conflicts | ||
const workerPort = | ||
command.includes(`--port`) || !command.startsWith("wrangler dev") | ||
? "" | ||
: " --port 0"; | ||
return `${WRANGLER} ${command.slice("wrangler ".length)}${inspectorPort}${workerPort}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
CLOUDFLARE_ACCOUNT_ID | ||
? [ | ||
{ imagesMode: "remote", extraFlags: "" }, | ||
{ | ||
imagesMode: "local", | ||
extraFlags: "--experimental-images-local-mode", | ||
}, | ||
] | ||
: [ | ||
{ | ||
imagesMode: "local", | ||
extraFlags: "--experimental-images-local-mode", | ||
}, | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLOUDFLARE_ACCOUNT_ID | |
? [ | |
{ imagesMode: "remote", extraFlags: "" }, | |
{ | |
imagesMode: "local", | |
extraFlags: "--experimental-images-local-mode", | |
}, | |
] | |
: [ | |
{ | |
imagesMode: "local", | |
extraFlags: "--experimental-images-local-mode", | |
}, | |
] | |
[ | |
... (CLOUDFLARE_ACCOUNT_ID ? [{ imagesMode: "remote", extraFlags: "" }] : []), | |
{ | |
imagesMode: "local", | |
extraFlags: "--experimental-images-local-mode", | |
}, | |
] |
packages/wrangler/e2e/dev.test.ts
Outdated
describe.each( | ||
CLOUDFLARE_ACCOUNT_ID | ||
? [{ cmd: "wrangler dev" }, { cmd: "wrangler dev --remote" }] | ||
: [{ cmd: "wrangler dev" }] | ||
)("basic js dev: $cmd", ({ cmd }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe.each( | |
CLOUDFLARE_ACCOUNT_ID | |
? [{ cmd: "wrangler dev" }, { cmd: "wrangler dev --remote" }] | |
: [{ cmd: "wrangler dev" }] | |
)("basic js dev: $cmd", ({ cmd }) => { | |
describe.each([ | |
... (CLOUDFLARE_ACCOUNT_ID ? [{ cmd: "wrangler dev --remote" }] : []), | |
{ cmd: "wrangler dev" } | |
])("basic js dev: $cmd", ({ cmd }) => { |
const RUNTIMES = CLOUDFLARE_ACCOUNT_ID | ||
? [ | ||
{ flags: "--remote", runtime: "remote" }, | ||
{ flags: "", runtime: "local" }, | ||
] | ||
: [{ flags: "", runtime: "local" }]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const RUNTIMES = CLOUDFLARE_ACCOUNT_ID | |
? [ | |
{ flags: "--remote", runtime: "remote" }, | |
{ flags: "", runtime: "local" }, | |
] | |
: [{ flags: "", runtime: "local" }]; | |
const RUNTIMES = [ | |
...(CLOUDFLARE_ACCOUNT_ID ? [{ flags: "--remote", runtime: "remote" }] : []), | |
{ flags: "", runtime: "local" }, | |
] |
const OPTIONS = CLOUDFLARE_ACCOUNT_ID | ||
? [{ remote: false }, { remote: true }] | ||
: [{ remote: false }]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
etc
process.env.WRANGLER ??= `node --no-warnings ${process.cwd()}/packages/wrangler/bin/wrangler.js`; | ||
process.env.WRANGLER_IMPORT ??= `${process.cwd()}/packages/wrangler/wrangler-dist/cli.js`; | ||
process.env.MINIFLARE_IMPORT ??= `${process.cwd()}/packages/miniflare/dist/src/index.js`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I think we should probably just hard code these into the tests now.
We only had them before because we sometimes tested against beta or production versions of Wrangler, which we don't do now.
In a follow up...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's hope no one just provides a CLOUDFLARE_ACCOUNT_ID and not a CLOUDFLARE_API_TOKEN.
A large number of Wrangler's "E2E" tests actually require no Cloudflare auth tokens. This PR changes our E2E setup to enable running
pnpm test:e2e:wrangler
with no other arguments, which will run all non-API-requiring E2E tests, including on forks in CI.Additionally, you can pass arguments to
pnpm test:e2e:wrangler
in order to configure Vitest. e.g.pnpm test:e2e:wrangler -u
to update snapshots.