Skip to content

Commit 80c91a7

Browse files
petebacondarwinpetebacondarwin
authored
investigate configcontroller test flakes (#10862)
* remove no compat date warnings * hide unwanted logging from r2 bucket test * Ensure that process.on("exit") handlers get removed This was causing lots of warnings in tests since we were just adding more and more handlers * teardown LocalRuntimeController in tests before removing tmp dir * centralize teardown handling and suppress error events after teardown has begun * move troublesome test to the start of the run * add logging to errors
1 parent 33b6929 commit 80c91a7

File tree

12 files changed

+116
-85
lines changed

12 files changed

+116
-85
lines changed

‎packages/wrangler/src/__tests__/api/startDevWorker/ConfigController.test.ts‎

Lines changed: 57 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@ import dedent from "ts-dedent";
44
import { describe, it } from "vitest";
55
import { ConfigController } from "../../../api/startDevWorker/ConfigController";
66
import { unwrapHook } from "../../../api/startDevWorker/utils";
7+
import { logger } from "../../../logger";
78
import { mockAccountId, mockApiToken } from "../../helpers/mock-account-id";
89
import { mockConsoleMethods } from "../../helpers/mock-console";
910
import { runInTempDir } from "../../helpers/run-in-tmp";
1011
import { seed } from "../../helpers/seed";
11-
import type { ConfigUpdateEvent, StartDevWorkerInput } from "../../../api";
12+
import type { ConfigUpdateEvent } from "../../../api";
1213

1314
async function waitForConfigUpdate(
1415
controller: ConfigController
@@ -34,8 +35,52 @@ describe("ConfigController", () => {
3435
let controller: ConfigController;
3536
beforeEach(() => {
3637
controller = new ConfigController();
38+
logger.loggerLevel = "debug";
39+
});
40+
afterEach(async () => {
41+
logger.debug("tearing down");
42+
await controller.teardown();
43+
logger.debug("teardown complete");
44+
logger.resetLoggerLevel();
45+
});
46+
47+
it("should use account_id from config file before env var", async () => {
48+
await seed({
49+
"src/index.ts": dedent/* javascript */ `
50+
export default {}
51+
`,
52+
"wrangler.toml": dedent/* toml */ `
53+
name = "my-worker"
54+
main = "src/index.ts"
55+
compatibility_date = \"2024-06-01\"
56+
`,
57+
});
58+
59+
const event = waitForConfigUpdate(controller);
60+
await controller.set({ config: "./wrangler.toml" });
61+
62+
const { config } = await event;
63+
await expect(unwrapHook(config.dev.auth)).resolves.toMatchObject({
64+
accountId: "some-account-id",
65+
apiToken: { apiToken: "some-api-token" },
66+
});
67+
68+
const event2 = waitForConfigUpdate(controller);
69+
await seed({
70+
"wrangler.toml": dedent/* toml */ `
71+
name = "my-worker"
72+
main = "src/index.ts"
73+
compatibility_date = \"2024-06-01\"
74+
account_id = "1234567890"
75+
`,
76+
});
77+
78+
const { config: config2 } = await event2;
79+
await expect(unwrapHook(config2.dev.auth)).resolves.toMatchObject({
80+
accountId: "1234567890",
81+
apiToken: { apiToken: "some-api-token" },
82+
});
3783
});
38-
afterEach(() => controller.teardown());
3984

4085
it("should emit configUpdate events with defaults applied", async () => {
4186
const event = waitForConfigUpdate(controller);
@@ -48,11 +93,10 @@ describe("ConfigController", () => {
4893
} satisfies ExportedHandler
4994
`,
5095
});
51-
const config: StartDevWorkerInput = {
52-
entrypoint: "src/index.ts",
53-
};
5496

55-
await controller.set(config);
97+
await controller.set({
98+
entrypoint: "src/index.ts",
99+
});
56100

57101
await expect(event).resolves.toMatchObject({
58102
type: "configUpdate",
@@ -82,12 +126,11 @@ describe("ConfigController", () => {
82126
`,
83127
"wrangler.toml": dedent`
84128
main = \"./some/base_dir/nested/index.js\"
85-
base_dir = \"./some/base_dir\"`,
129+
compatibility_date = \"2024-06-01\"
130+
base_dir = \"./some/base_dir\"`,
86131
});
87132

88-
const config: StartDevWorkerInput = {};
89-
90-
await controller.set(config);
133+
await controller.set({});
91134

92135
await expect(event).resolves.toMatchObject({
93136
type: "configUpdate",
@@ -104,6 +147,7 @@ base_dir = \"./some/base_dir\"`,
104147
},
105148
});
106149
});
150+
107151
it("should shallow merge patched config", async () => {
108152
const event1 = waitForConfigUpdate(controller);
109153
await seed({
@@ -115,11 +159,10 @@ base_dir = \"./some/base_dir\"`,
115159
} satisfies ExportedHandler
116160
`,
117161
});
118-
const config: StartDevWorkerInput = {
119-
entrypoint: "src/index.ts",
120-
};
121162

122-
await controller.set(config);
163+
await controller.set({
164+
entrypoint: "src/index.ts",
165+
});
123166

124167
await expect(event1).resolves.toMatchObject({
125168
type: "configUpdate",
@@ -196,41 +239,4 @@ base_dir = \"./some/base_dir\"`,
196239
},
197240
});
198241
});
199-
200-
it("should use account_id from config file before env var", async () => {
201-
await seed({
202-
"src/index.ts": dedent/* javascript */ `
203-
export default {}
204-
`,
205-
"wrangler.toml": dedent/* toml */ `
206-
name = "my-worker"
207-
main = "src/index.ts"
208-
`,
209-
});
210-
211-
const event = waitForConfigUpdate(controller);
212-
await controller.set({ config: "./wrangler.toml" });
213-
214-
const { config } = await event;
215-
await expect(unwrapHook(config.dev.auth)).resolves.toMatchObject({
216-
accountId: "some-account-id",
217-
apiToken: { apiToken: "some-api-token" },
218-
});
219-
220-
const event2 = waitForConfigUpdate(controller);
221-
await seed({
222-
"wrangler.toml": dedent/* toml */ `
223-
name = "my-worker"
224-
main = "src/index.ts"
225-
account_id = "1234567890"
226-
`,
227-
});
228-
await controller.set({ config: "./wrangler.toml" });
229-
230-
const { config: config2 } = await event2;
231-
await expect(unwrapHook(config2.dev.auth)).resolves.toMatchObject({
232-
accountId: "1234567890",
233-
apiToken: { apiToken: "some-api-token" },
234-
});
235-
});
236242
});

‎packages/wrangler/src/__tests__/api/startDevWorker/LocalRuntimeController.test.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,10 @@ function configDefaults(
138138
}
139139

140140
describe("LocalRuntimeController", () => {
141-
const teardown = useTeardown();
142141
mockConsoleMethods();
143142
runInTempDir();
143+
// Make sure teardown is declared after runInTempDir so it runs before we delete the temp directory
144+
const teardown = useTeardown();
144145

145146
describe("Core", () => {
146147
it("should start Miniflare with module worker", async () => {

‎packages/wrangler/src/__tests__/dev/remote-bindings.test.ts‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,10 @@ vi.mock("../../dev/start-dev", async () => {
3131
async startDev(args: StartDevOptions) {
3232
const result = await actual.startDev(args);
3333
stopWrangler = () => {
34-
try {
35-
return result.devEnv.teardown();
36-
} finally {
37-
stopWrangler = async () => {
38-
throw new Error("Stop worker already called");
39-
};
40-
}
34+
stopWrangler = async () => {
35+
throw new Error("Stop worker already called");
36+
};
37+
return result.devEnv.teardown();
4138
};
4239
return result;
4340
},

‎packages/wrangler/src/__tests__/r2/bucket.test.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@ const mockGetR2BucketMetrics = vi.mocked(getR2BucketMetrics);
1515

1616
describe("r2 bucket info", () => {
1717
beforeEach(() => {
18-
vi.resetAllMocks();
19-
2018
mockRequireAuth.mockResolvedValue("test-account-id");
2119
mockGetR2Bucket.mockResolvedValue({
2220
name: "my-bucket-name",

‎packages/wrangler/src/api/startDevWorker/BaseController.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { EventEmitter } from "node:events";
2+
import { logger } from "../../logger";
23
import type {
34
BundleCompleteEvent,
45
BundleStartEvent,
@@ -68,8 +69,18 @@ export type ControllerEventMap = {
6869
export abstract class Controller<
6970
EventMap extends ControllerEventMap = ControllerEventMap,
7071
> extends TypedEventEmitterImpl<EventMap> {
71-
emitErrorEvent(data: ErrorEvent) {
72-
this.emit("error", data);
72+
#tearingDown = false;
73+
async teardown(): Promise<void> {
74+
this.#tearingDown = true;
75+
}
76+
emitErrorEvent(event: ErrorEvent) {
77+
if (this.#tearingDown) {
78+
logger.debug("Suppressing error event during teardown");
79+
logger.debug(`Error in ${event.source}: ${event.reason}\n`, event.cause);
80+
logger.debug("=> Error contextual data:", event.data);
81+
return;
82+
}
83+
this.emit("error", event);
7384
}
7485
}
7586

@@ -86,7 +97,6 @@ export abstract class RuntimeController extends Controller<RuntimeControllerEven
8697
abstract onBundleStart(_: BundleStartEvent): void;
8798
abstract onBundleComplete(_: BundleCompleteEvent): void;
8899
abstract onPreviewTokenExpired(_: PreviewTokenExpiredEvent): void;
89-
abstract teardown(): Promise<void>;
90100

91101
// *********************
92102
// Event Dispatchers

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,8 +374,9 @@ export class BundlerController extends Controller<BundlerControllerEventMap> {
374374
});
375375
}
376376

377-
async teardown() {
377+
override async teardown() {
378378
logger.debug("BundlerController teardown beginning...");
379+
await super.teardown();
379380
this.#customBuildAborter?.abort();
380381
this.#tmpDir?.remove();
381382
await Promise.all([

‎packages/wrangler/src/api/startDevWorker/ConfigController.ts‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,6 @@ export class ConfigController extends Controller<ConfigControllerEventMap> {
488488

489489
#configWatcher?: ReturnType<typeof watch>;
490490
#abortController?: AbortController;
491-
#tearingDown = false;
492491

493492
async #ensureWatchingConfig(configPath: string | undefined) {
494493
await this.#configWatcher?.close();
@@ -505,17 +504,28 @@ export class ConfigController extends Controller<ConfigControllerEventMap> {
505504
this.latestInput,
506505
"Cannot be watching config without having first set an input"
507506
);
508-
void this.#updateConfig(this.latestInput);
507+
logger.debug("config file changed", configPath);
508+
this.#updateConfig(this.latestInput).catch((err) => {
509+
this.emitErrorEvent({
510+
type: "error",
511+
reason: "Error resolving config after change",
512+
cause: castErrorCause(err),
513+
source: "ConfigController",
514+
data: undefined,
515+
});
516+
});
509517
});
510518
}
511519
}
512520

513521
public set(input: StartDevWorkerInput, throwErrors = false) {
522+
logger.debug("setting config");
514523
return runWithLogLevel(input.dev?.logLevel, () =>
515524
this.#updateConfig(input, throwErrors)
516525
);
517526
}
518527
public patch(input: Partial<StartDevWorkerInput>) {
528+
logger.debug("patching config");
519529
assert(
520530
this.latestInput,
521531
"Cannot call updateConfig without previously calling setConfig"
@@ -532,9 +542,11 @@ export class ConfigController extends Controller<ConfigControllerEventMap> {
532542
}
533543

534544
async #updateConfig(input: StartDevWorkerInput, throwErrors = false) {
535-
if (this.#tearingDown) {
536-
return;
537-
}
545+
logger.debug(
546+
"Updating config...",
547+
this.#abortController?.signal,
548+
this.#configWatcher?.closed
549+
);
538550
this.#abortController?.abort();
539551
this.#abortController = new AbortController();
540552
const signal = this.#abortController.signal;
@@ -568,14 +580,10 @@ export class ConfigController extends Controller<ConfigControllerEventMap> {
568580
await this.#ensureWatchingConfig(fileConfig.configPath);
569581
}
570582

571-
if (this.#tearingDown || signal.aborted) {
572-
return;
573-
}
574-
575583
const { config: resolvedConfig, printCurrentBindings } =
576584
await resolveConfig(fileConfig, input);
577585

578-
if (this.#tearingDown || signal.aborted) {
586+
if (signal.aborted) {
579587
return;
580588
}
581589
this.latestConfig = resolvedConfig;
@@ -584,9 +592,14 @@ export class ConfigController extends Controller<ConfigControllerEventMap> {
584592

585593
return this.latestConfig;
586594
} catch (err) {
595+
logger.debug("Error updating config", (err as Error).stack);
587596
if (throwErrors) {
588597
throw err;
589598
} else {
599+
if (this.#configWatcher?.closed) {
600+
logger.debug("Suppressing config error after watcher closed");
601+
return;
602+
}
590603
this.emitErrorEvent({
591604
type: "error",
592605
reason: "Error resolving config",
@@ -606,9 +619,9 @@ export class ConfigController extends Controller<ConfigControllerEventMap> {
606619
this.#printCurrentBindings?.(event.registry);
607620
}
608621

609-
async teardown() {
622+
override async teardown() {
610623
logger.debug("ConfigController teardown beginning...");
611-
this.#tearingDown = true;
624+
await super.teardown();
612625
this.#abortController?.abort();
613626
await this.#configWatcher?.close();
614627
logger.debug("ConfigController teardown complete");

‎packages/wrangler/src/api/startDevWorker/LocalRuntimeController.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,9 +194,9 @@ export class LocalRuntimeController extends RuntimeController {
194194
};
195195

196196
onBundleStart(_: BundleStartEvent) {
197-
process.on("exit", () => {
198-
this.cleanupContainers();
199-
});
197+
// Remove any existing listener, then add a new one.
198+
process.off("exit", this.cleanupContainers);
199+
process.on("exit", this.cleanupContainers);
200200
}
201201

202202
async #onBundleComplete(data: BundleCompleteEvent, id: number) {
@@ -416,6 +416,8 @@ export class LocalRuntimeController extends RuntimeController {
416416
#teardown = async (): Promise<void> => {
417417
logger.debug("LocalRuntimeController teardown beginning...");
418418

419+
process.off("exit", this.cleanupContainers);
420+
419421
if (this.#mf) {
420422
logger.log(chalk.dim("⎔ Shutting down local server..."));
421423
}
@@ -432,7 +434,8 @@ export class LocalRuntimeController extends RuntimeController {
432434

433435
logger.debug("LocalRuntimeController teardown complete");
434436
};
435-
async teardown() {
437+
override async teardown() {
438+
await super.teardown();
436439
return this.#mutex.runWith(this.#teardown);
437440
}
438441

‎packages/wrangler/src/api/startDevWorker/MultiworkerRuntimeController.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,8 @@ export class MultiworkerRuntimeController extends LocalRuntimeController {
313313

314314
logger.debug("MultiworkerRuntimeController teardown complete");
315315
};
316-
async teardown() {
316+
override async teardown() {
317+
await super.teardown();
317318
return this.#mutex.runWith(this.#teardown);
318319
}
319320
}

‎packages/wrangler/src/api/startDevWorker/NoOpProxyController.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,5 +11,4 @@ export class NoOpProxyController extends ProxyController {
1111
onBundleStart(_data: BundleStartEvent) {}
1212
onReloadStart(_data: ReloadStartEvent) {}
1313
onReloadComplete(_data: ReloadCompleteEvent) {}
14-
async teardown() {}
1514
}

0 commit comments

Comments
 (0)