improve(wrangler): fix subdomain tests (#7210)

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.

Author: taylorlee

.changeset/clean-tigers-breathe.md

@@ -0,0 +1,5 @@
+---
+"wrangler": patch
+---
+
+Avoid an unnecessary GET request during `wrangler deploy`.

packages/wrangler/src/__tests__/deploy.test.ts

@@ -1,4 +1,5 @@
 import { http, HttpResponse } from "msw";
+import { mockGetWorkerSubdomain } from "./mock-workers-subdomain";
 import { createFetchResult, msw } from "./msw";
 import { serialize, toString } from "./serialize-form-data-entry";
 import type { WorkerMetadata } from "../../deployment-bundle/create-worker-upload-form";
@@ -10,7 +11,6 @@ import type { HttpResponseResolver } from "msw";
 /** Create a mock handler for the request to upload a worker script. */
 export function mockUploadWorkerRequest(
 	options: {
-		available_on_subdomain?: boolean;
 		expectedEntry?: string | RegExp | ((entry: string | null) => void);
 		expectedMainModule?: string;
 		expectedType?: "esm" | "sw" | "none";
@@ -50,9 +50,6 @@ export function mockUploadWorkerRequest(
 			expect(params.envName).toEqual(env);
 		}
 		if (useOldUploadApi) {
-			expect(url.searchParams.get("include_subdomain_availability")).toEqual(
-				"true"
-			);
 			expect(url.searchParams.get("excludeScript")).toEqual("true");
 		}
 		if (expectedDispatchNamespace) {
@@ -130,7 +127,6 @@ export function mockUploadWorkerRequest(
 		if (useOldUploadApi) {
 			return HttpResponse.json(
 				createFetchResult({
-					available_on_subdomain,
 					id: "abc12345",
 					etag: "etag98765",
 					pipeline_hash: "hash9999",
@@ -156,7 +152,6 @@ export function mockUploadWorkerRequest(
 	};
 
 	const {
-		available_on_subdomain = true,
 		expectedEntry,
 		expectedAssets,
 		// Allow setting expectedMainModule to undefined to test static-asset only uploads
@@ -227,25 +222,11 @@ export function mockUploadWorkerRequest(
 			)
 		);
 	}
-
-	msw.use(
-		http.get(
-			env && !legacyEnv
-				? `*/accounts/:accountId/workers/services/:scriptName/environments/:envName/subdomain`
-				: `*/accounts/:accountId/workers/scripts/:scriptName/subdomain`,
-			({ params }) => {
-				expect(params.accountId).toEqual("some-account-id");
-				expect(params.scriptName).toEqual(
-					legacyEnv && env ? `${expectedScriptName}-${env}` : expectedScriptName
-				);
-				if (!legacyEnv) {
-					expect(params.envName).toEqual(env);
-				}
-
-				return HttpResponse.json(
-					createFetchResult({ enabled: available_on_subdomain })
-				);
-			}
-		)
-	);
+	// TODO make explicit by callers?
+	mockGetWorkerSubdomain({
+		enabled: true,
+		env,
+		legacyEnv,
+		expectedScriptName,
+	});
 }

packages/wrangler/src/__tests__/helpers/mock-upload-worker.ts

@@ -32,3 +32,74 @@ export function mockSubDomainRequest(
 		);
 	}
 }
+
+/** Create a mock handler to fetch the  <script>.<user>.workers.dev subdomain status*/
+export function mockGetWorkerSubdomain({
+	enabled,
+	env,
+	legacyEnv = false,
+	expectedScriptName = "test-name",
+}: {
+	enabled: boolean;
+	env?: string | undefined;
+	legacyEnv?: boolean | undefined;
+	expectedScriptName?: string;
+}) {
+	const url =
+		env && !legacyEnv
+			? `*/accounts/:accountId/workers/services/:scriptName/environments/:envName/subdomain`
+			: `*/accounts/:accountId/workers/scripts/:scriptName/subdomain`;
+	msw.use(
+		http.get(
+			url,
+			({ params }) => {
+				expect(params.accountId).toEqual("some-account-id");
+				expect(params.scriptName).toEqual(
+					legacyEnv && env ? `${expectedScriptName}-${env}` : expectedScriptName
+				);
+				if (!legacyEnv) {
+					expect(params.envName).toEqual(env);
+				}
+
+				return HttpResponse.json(createFetchResult({ enabled }));
+			},
+			{ once: true }
+		)
+	);
+}
+
+/** Create a mock handler to toggle a <script>.<user>.workers.dev subdomain status */
+export function mockUpdateWorkerSubdomain({
+	enabled,
+	env,
+	legacyEnv = false,
+	expectedScriptName = "test-name",
+}: {
+	enabled: boolean;
+	env?: string | undefined;
+	legacyEnv?: boolean | undefined;
+	expectedScriptName?: string;
+}) {
+	const url =
+		env && !legacyEnv
+			? `*/accounts/:accountId/workers/services/:scriptName/environments/:envName/subdomain`
+			: `*/accounts/:accountId/workers/scripts/:scriptName/subdomain`;
+	msw.use(
+		http.post(
+			url,
+			async ({ request, params }) => {
+				expect(params.accountId).toEqual("some-account-id");
+				expect(params.scriptName).toEqual(
+					legacyEnv && env ? `${expectedScriptName}-${env}` : expectedScriptName
+				);
+				if (!legacyEnv) {
+					expect(params.envName).toEqual(env);
+				}
+				const body = await request.json();
+				expect(body).toEqual({ enabled });
+				return HttpResponse.json(createFetchResult({ enabled }));
+			},
+			{ once: true }
+		)
+	);
+}

packages/wrangler/src/__tests__/helpers/mock-workers-subdomain.ts

@@ -12,7 +12,10 @@ import { mockAccountId, mockApiToken } from "../helpers/mock-account-id";
 import { mockConsoleMethods } from "../helpers/mock-console";
 import { useMockIsTTY } from "../helpers/mock-istty";
 import { mockUploadWorkerRequest } from "../helpers/mock-upload-worker";
-import { mockSubDomainRequest } from "../helpers/mock-workers-subdomain";
+import {
+	mockGetWorkerSubdomain,
+	mockSubDomainRequest,
+} from "../helpers/mock-workers-subdomain";
 import {
 	msw,
 	mswGetVersion,
@@ -57,8 +60,9 @@ describe("versions deploy", () => {
 			);
 			writeWranglerToml();
 			writeWorkerSource();
-			mockSubDomainRequest();
 			mockUploadWorkerRequest();
+			mockGetWorkerSubdomain({ enabled: true });
+			mockSubDomainRequest();
 
 			await runWrangler("deploy ./index");
 

packages/wrangler/src/__tests__/versions/versions.deploy.test.ts

@@ -2,7 +2,10 @@ import { http, HttpResponse } from "msw";
 import { mockAccountId, mockApiToken } from "../helpers/mock-account-id";
 import { mockConsoleMethods } from "../helpers/mock-console";
 import { useMockIsTTY } from "../helpers/mock-istty";
-import { mockSubDomainRequest } from "../helpers/mock-workers-subdomain";
+import {
+	mockGetWorkerSubdomain,
+	mockSubDomainRequest,
+} from "../helpers/mock-workers-subdomain";
 import { createFetchResult, msw } from "../helpers/msw";
 import { runInTempDir } from "../helpers/run-in-tmp";
 import { runWrangler } from "../helpers/run-wrangler";
@@ -21,7 +24,7 @@ describe("versions upload", () => {
 			http.get(
 				`*/accounts/:accountId/workers/services/:scriptName`,
 				({ params }) => {
-					expect(params.scriptName).toEqual("test-worker");
+					expect(params.scriptName).toEqual("test-name");
 
 					return HttpResponse.json(
 						createFetchResult({
@@ -47,7 +50,7 @@ describe("versions upload", () => {
 						return HttpResponse.error();
 					}
 
-					expect(params.scriptName).toEqual("test-worker");
+					expect(params.scriptName).toEqual("test-name");
 
 					return HttpResponse.json(
 						createFetchResult({
@@ -63,27 +66,13 @@ describe("versions upload", () => {
 		);
 	}
 
-	function mockGetWorkerSubdomain(available_on_subdomain: boolean) {
-		msw.use(
-			http.get(
-				`*/accounts/:accountId/workers/scripts/:scriptName/subdomain`,
-				({ params }) => {
-					expect(params.scriptName).toEqual("test-worker");
-					return HttpResponse.json(
-						createFetchResult({ enabled: available_on_subdomain })
-					);
-				}
-			)
-		);
-	}
-
 	test("should print bindings & startup time on versions upload", async () => {
 		mockGetScript();
 		mockUploadVersion(false);
 
 		// Setup
 		writeWranglerToml({
-			name: "test-worker",
+			name: "test-name",
 			main: "./index.js",
 			vars: {
 				TEST: "test-string",
@@ -113,20 +102,20 @@ describe("versions upload", () => {
 			 \\"abc\\": \\"def\\",
 			 \\"bool\\": true
 			}
-			Uploaded test-worker (TIMINGS)
+			Uploaded test-name (TIMINGS)
 			Worker Version ID: 51e4886e-2db7-4900-8d38-fbfecfeab993"
 		`);
 	});
 
 	test("should print preview url if version has preview", async () => {
 		mockGetScript();
 		mockUploadVersion(true);
-		mockGetWorkerSubdomain(true);
+		mockGetWorkerSubdomain({ enabled: true });
 		mockSubDomainRequest();
 
 		// Setup
 		writeWranglerToml({
-			name: "test-worker",
+			name: "test-name",
 			main: "./index.js",
 			vars: {
 				TEST: "test-string",
@@ -145,20 +134,20 @@ describe("versions upload", () => {
 			Your worker has access to the following bindings:
 			- Vars:
 			  - TEST: \\"test-string\\"
-			Uploaded test-worker (TIMINGS)
+			Uploaded test-name (TIMINGS)
 			Worker Version ID: 51e4886e-2db7-4900-8d38-fbfecfeab993
-			Version Preview URL: https://51e4886e-test-worker.test-sub-domain.workers.dev"
+			Version Preview URL: https://51e4886e-test-name.test-sub-domain.workers.dev"
 		`);
 	});
 
 	it("should not print preview url workers_dev is false", async () => {
 		mockGetScript();
 		mockUploadVersion(true);
-		mockGetWorkerSubdomain(false);
+		mockGetWorkerSubdomain({ enabled: false });
 
 		// Setup
 		writeWranglerToml({
-			name: "test-worker",
+			name: "test-name",
 			main: "./index.js",
 			vars: {
 				TEST: "test-string",
@@ -177,7 +166,7 @@ describe("versions upload", () => {
 			Your worker has access to the following bindings:
 			- Vars:
 			  - TEST: \\"test-string\\"
-			Uploaded test-worker (TIMINGS)
+			Uploaded test-name (TIMINGS)
 			Worker Version ID: 51e4886e-2db7-4900-8d38-fbfecfeab993"
 		`);
 	});

packages/wrangler/src/__tests__/versions/versions.upload.test.ts

@@ -813,10 +813,8 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m
 			let bindingsPrinted = false;
 
 			// Upload the script so it has time to propagate.
-			// We can also now tell whether available_on_subdomain is set
 			try {
 				let result: {
-					available_on_subdomain: boolean;
 					id: string | null;
 					etag: string | null;
 					pipeline_hash: string | null;
@@ -854,12 +852,7 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m
 						observability: worker.observability ?? { enabled: false },
 					});
 
-					const { available_on_subdomain } = await fetchResult<{
-						available_on_subdomain: boolean;
-					}>(`/accounts/${accountId}/workers/scripts/${scriptName}/subdomain`);
-
 					result = {
-						available_on_subdomain,
 						id: null, // fpw - ignore
 						etag: versionResult.resources.script.etag,
 						pipeline_hash: null, // fpw - ignore
@@ -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;
 							id: string | null;
 							etag: string | null;
 							pipeline_hash: string | null;
@@ -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",
 								// pass excludeScript so the whole body of the
 								// script doesn't get included in the response
 								excludeScript: "true",