improve(wrangler): fix subdomain tests #7210
Conversation
Many tests had mocks that weren't called, or had false assertions about not updating subdomain routes when they actually were being updated. Additionally, available_on_subdomain is now unecessary during script-upload, as it is not read there, due to being handled entirely within triggers-deploy. This also saves a double call to get /subdomain in the new-api path. The get/update subdomain helpers are refactored from deploy/version tests, and unified to standardize default workername and legacyEnv. I wanted to remove the getSubdomain mock from mockUpload entirely, but there were a bit too many callsites... At least every test now that makes assertions about workers.dev deployments does explicitly reset the mock return value.
🦋 Changeset detectedLatest commit: 9ae9c6c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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/11759185956/npm-package-wrangler-7210You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7210/npm-package-wrangler-7210Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11759185956/npm-package-wrangler-7210 dev path/to/script.jsAdditional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11759185956/npm-package-create-cloudflare-7210 --no-auto-updatenpm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11759185956/npm-package-cloudflare-kv-asset-handler-7210npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11759185956/npm-package-miniflare-7210npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11759185956/npm-package-cloudflare-pages-shared-7210npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11759185956/npm-package-cloudflare-vitest-pool-workers-7210npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11759185956/npm-package-cloudflare-workers-editor-shared-7210npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11759185956/npm-package-cloudflare-workers-shared-7210npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11759185956/npm-package-cloudflare-workflows-shared-7210Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
| } | ||
| ) | ||
| ); | ||
| // TODO make explicit by callers? |
There was a problem hiding this comment.
TODO in this PR? Otherwise should we track this somewhere?
There was a problem hiding this comment.
Future thing, I'm not sure where is best to track
| @@ -885,7 +877,6 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m | |||
| headers: await getMetricsUsageHeaders(config.send_metrics), | |||
| }, | |||
| new URLSearchParams({ | |||
| include_subdomain_availability: "true", | |||
There was a problem hiding this comment.
should we remove
as well?There was a problem hiding this comment.
I'll not remove for now just to keep the scope a bit smaller but yeah, it isn't needed here either
| @@ -870,7 +863,6 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m | |||
| } else { | |||
| result = await retryOnError(async () => | |||
| fetchResult<{ | |||
| available_on_subdomain: boolean; | |||
There was a problem hiding this comment.
sanity check...is this the only place available_on_subdomain needs to be removed from the fetch result shape? Seeing it in other places too, such as versions upload, and triggers...tho my understanding is that triggers has it right and it should be kept there
There was a problem hiding this comment.
Yeah for scoping I think good to only remove in this path right now. Deploy is what was using it when it wasn't needed and what was cleaned up. Will make a note to cleanup the rest though
Many tests had mocks that weren't called, or had false assertions about not updating subdomain routes when they actually were being updated.
Additionally, available_on_subdomain is now unecessary during script-upload, as it is not read there, due to being handled entirely within triggers-deploy. This also saves a double call to get /subdomain in the new-api path.
The get/update subdomain helpers are refactored from deploy/version tests, and unified to standardize default workername and legacyEnv.
I wanted to remove the getSubdomain mock from mockUpload entirely, but there were a bit too many callsites... At least every test now that makes assertions about workers.dev deployments does explicitly reset the mock return value.
Fixes N/A