file based registry fixtures #6893

RamIdeas · 2024-10-04T11:55:17Z

What this PR solves / how to test

Fixes N/A

This PR removes flaky dev registry fixtures, and shifts them to be run in Wrangler's e2e test suite to improve reliability and reduce the number of times they're run.

Author has addressed the following

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:
Changeset (Changeset guidelines)
- TODO (before merge)
- Changeset included
- Changeset not necessary because: shouldn't trigger a release
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Documentation not necessary because: internal workflows change

changeset-bot · 2024-10-04T11:55:21Z

⚠️ No Changeset found

Latest commit: 66b0916

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

github-actions · 2024-10-04T11:57:53Z

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/11327304683/npm-package-wrangler-6893

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11327304683/npm-package-wrangler-6893 dev path/to/script.js

Additional artifacts:

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

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

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

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

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

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

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

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

wrangler@3.80.4 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20241004.0
`workerd`	1.20241004.0	1.20241004.0
`workerd --version`	1.20241004.0	2024-10-04

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

penalosa · 2024-10-10T13:21:25Z

fixtures/entrypoints-rpc-tests/tests/entrypoints.spec.ts

The changes in this file are to support running with --x-registry

penalosa · 2024-10-10T13:21:55Z

fixtures/get-platform-proxy/tests/get-platform-proxy.env.test.ts

@@ -150,92 +115,6 @@ describe("getPlatformProxy - env", () => {
 		}
 	});

-	it("provides service bindings to external local workers", async () => {


These are now tested in the get-platform-proxy.test.ts e2e test

penalosa · 2024-10-10T13:22:49Z

packages/wrangler/e2e/dev.test.ts

@@ -221,107 +216,6 @@ describe.each([
 	});
 });

-describe.each([


Moved to dev-registry.test.ts

penalosa · 2024-10-10T13:24:44Z

packages/wrangler/e2e/helpers/command.ts

-			if (stderr.length) {
-				console.error(stderr);
-			}
+		if (stdout.length) {
+			console.log(`[${path.basename(cwd ?? "/unknown")}]`, stdout);
 		}
+		if (stderr.length) {
+			console.error(`[${path.basename(cwd ?? "/unknown")}]`, stderr);
+		}


This means our E2E test output will be a lot noisier, but I think we should keep logging enabled by default for now to help us diagnose and fix flakes

penalosa · 2024-10-10T13:24:50Z

packages/wrangler/e2e/helpers/command.ts

-					if (process.env.VITEST_MODE === "WATCH") {
-						console.log(line);
-					}
+					console.log(`[${path.basename(cwd ?? "/unknown")}]`, line);


penalosa · 2024-10-10T13:26:19Z

packages/wrangler/src/api/startDevWorker/BundlerController.ts

+				if (!buildAborter.signal.aborted) {
+					this.emitBundleCompleteEvent(config, newBundle);
+					this.#currentBundle = newBundle;
+				}
 			},
-			(err) =>
-				this.emitErrorEvent({
-					type: "error",
-					reason: "Failed to construct initial bundle",
-					cause: castErrorCause(err),
-					source: "BundlerController",
-					data: undefined,
-				})
+			(err) => {
+				if (!buildAborter.signal.aborted) {
+					this.emitErrorEvent({
+						type: "error",
+						reason: "Failed to construct initial bundle",
+						cause: castErrorCause(err),
+						source: "BundlerController",
+						data: undefined,
+					});
+				}
+			}


BundleController had a race condition where old builds would sometimes overwrite newer ones. We'd thought this was handled by esbuild, but it turns out that it's not in the way we need it to. This fixes the intermittent "Failed to construct initial bundle" flakes we were seeing in E2E tests in CI

Very cool, thanks for that.

penalosa · 2024-10-10T13:26:56Z

packages/wrangler/src/config/index.ts

+						const registryDefinition = registry?.[script_name];
+						if (
+							registryDefinition &&
+							registryDefinition.durableObjects.some(
+								(d) => d.className === class_name
+							)
+						) {
+							value += ` (defined in 🟢 ${script_name})`;
+						} else {
+							value += ` (defined in 🔴 ${script_name})`;
+						}


This makes it much easier to see at a glance which service bindings are connected via the dev registry. Happy to bikeshed on the emoji!

andyjessop

Looks great! One question, but otherwise really happy this is going forward 🚀

andyjessop · 2024-10-11T15:02:13Z

fixtures/shared/src/run-wrangler-long-lived.ts

+				"dev",
+				publicPath,
+				"--x-dev-env",
+				"--x-registry",


andyjessop · 2024-10-11T15:07:50Z

packages/wrangler/src/api/startDevWorker/BundlerController.ts

+				if (!buildAborter.signal.aborted) {
+					this.emitBundleCompleteEvent(config, newBundle);
+					this.#currentBundle = newBundle;
+				}
 			},
-			(err) =>
-				this.emitErrorEvent({
-					type: "error",
-					reason: "Failed to construct initial bundle",
-					cause: castErrorCause(err),
-					source: "BundlerController",
-					data: undefined,
-				})
+			(err) => {
+				if (!buildAborter.signal.aborted) {
+					this.emitErrorEvent({
+						type: "error",
+						reason: "Failed to construct initial bundle",
+						cause: castErrorCause(err),
+						source: "BundlerController",
+						data: undefined,
+					});
+				}
+			}


Very cool, thanks for that.

andyjessop · 2024-10-11T15:08:39Z

packages/wrangler/src/dev.tsx

@@ -476,6 +476,11 @@ async function updateDevEnvRegistry(
 	devEnv: DevEnv,
 	registry: WorkerRegistry | undefined
 ) {
+	// Make sure we're not patching an empty config
+	if (!devEnv.config.latestConfig) {


What's this change fixing?

This is just moving this check higher up the function. Previously we got bound workers and then waited for a config update, but since getting bound workers depends on the config we should've been waiting here instead

Aha! Does this explain the race we were seeing?

One of them, yes! Another was in the bundle controller, and there might still be others lurking

packages/wrangler/e2e/dev-registry.test.ts

RamIdeas · 2024-10-14T11:02:00Z

packages/wrangler/src/dev.tsx

@@ -476,6 +476,11 @@ async function updateDevEnvRegistry(
 	devEnv: DevEnv,
 	registry: WorkerRegistry | undefined
 ) {
+	// Make sure we're not patching an empty config
+	if (!devEnv.config.latestConfig) {


Aha! Does this explain the race we were seeing?

packages/wrangler/src/config/index.ts

Co-authored-by: Rahul Sethi <5822355+RamIdeas@users.noreply.github.com>

RamIdeas mentioned this pull request Oct 4, 2024

Fix combo of --x-dev-env and --x-registry #6871

Closed

12 tasks

penalosa added the e2e Run e2e tests on a PR label Oct 4, 2024

penalosa force-pushed the fix-x-registry branch from 4f0abde to f8b18e5 Compare October 10, 2024 03:03

penalosa reviewed Oct 10, 2024

View reviewed changes

penalosa marked this pull request as ready for review October 10, 2024 13:45

penalosa requested a review from a team as a code owner October 10, 2024 13:45

penalosa force-pushed the fix-x-registry branch from ef4420c to 27a8d3b Compare October 10, 2024 17:39

andyjessop approved these changes Oct 11, 2024

View reviewed changes

RamIdeas and others added 20 commits October 11, 2024 16:23

wip

5f3d629

make get-platform-proxy unstable_dev serial not parallel

ab85085

repeats: 10

3bd1393

back to original retries: 2 + no repeats

aee5542

re-skip external-durable-object-app fixutre test

d881116

Expand registry e2e tests

21e5c82

remove flaky fixtures

8433f1e

log stack

71ebe38

Further moves to e2e

396c185

run less

c8fe691

change test

3e4bab9

only propogate errors if build not aborted

6c30f97

more logging

dafc7b2

add label

9f5c3b3

split up more

f4f3620

add more connection logging

45591c2

more robust matching

5da7347

remove ordering requirements

caba0b2

first time pass?

61df041

skip more windows

1e88cf7

penalosa added 14 commits October 11, 2024 16:24

debug log stack

90a22c0

more tests

7a452d2

snapshots

c1541e2

snapshots

6b5860a

optional tee

19ee28d

fix bundle controller race condition

70eadc2

tee opt-in

47de140

more fixes

d1fd7bc

no only

8b2ecd3

increase timeout

57fd9f6

readUntil -> waitFor

388a424

more logging

a75526f

increase timeout

1dea6af

no more teeing

6daac0c

penalosa force-pushed the fix-x-registry branch from 27a8d3b to 6daac0c Compare October 11, 2024 15:25

RamIdeas commented Oct 14, 2024

View reviewed changes

penalosa and others added 3 commits October 14, 2024 12:31

Apply suggestions from code review

754b2f1

Co-authored-by: Rahul Sethi <5822355+RamIdeas@users.noreply.github.com>

Update packages/wrangler/src/config/index.ts

5973742

Co-authored-by: Rahul Sethi <5822355+RamIdeas@users.noreply.github.com>

fix formatting

66b0916

penalosa merged commit 07fe80b into main Oct 14, 2024
22 checks passed

penalosa deleted the fix-x-registry branch October 14, 2024 14:19

isaac-mcfadyen mentioned this pull request Oct 17, 2024

FEEDBACK: red or green dot on DO services #7009

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

file based registry fixtures #6893

file based registry fixtures #6893

RamIdeas commented Oct 4, 2024 •

edited by penalosa

Loading

changeset-bot bot commented Oct 4, 2024 •

edited

Loading

github-actions bot commented Oct 4, 2024 •

edited

Loading

penalosa Oct 10, 2024

penalosa Oct 10, 2024

penalosa Oct 10, 2024

penalosa Oct 10, 2024

penalosa Oct 10, 2024

penalosa Oct 10, 2024

andyjessop Oct 11, 2024

penalosa Oct 10, 2024

andyjessop left a comment

andyjessop Oct 11, 2024

andyjessop Oct 11, 2024

andyjessop Oct 11, 2024

penalosa Oct 11, 2024

RamIdeas Oct 14, 2024

penalosa Oct 14, 2024

RamIdeas Oct 14, 2024

file based registry fixtures #6893

file based registry fixtures #6893

Conversation

RamIdeas commented Oct 4, 2024 • edited by penalosa Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Oct 4, 2024 • edited Loading

⚠️ No Changeset found

github-actions bot commented Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyjessop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamIdeas commented Oct 4, 2024 •

edited by penalosa

Loading

changeset-bot bot commented Oct 4, 2024 •

edited

Loading

github-actions bot commented Oct 4, 2024 •

edited

Loading