From bddc457003ec6c924a416c7f3358890b9c2fe881 Mon Sep 17 00:00:00 2001 From: Dobes Vandermeer Date: Mon, 31 Mar 2025 17:22:02 -0700 Subject: [PATCH 1/3] Fix issue with tests failing during cleanup The temp folder was deleted before the write stream would close, and it would cause node to exit due to an unhandled error. --- packages/cli/tests/initializeReporters.test.ts | 9 ++++++++- packages/reporters/src/ChromeTraceEventsReporter.ts | 13 +++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/packages/cli/tests/initializeReporters.test.ts b/packages/cli/tests/initializeReporters.test.ts index 4fa3c19c3..d221ba5d9 100644 --- a/packages/cli/tests/initializeReporters.test.ts +++ b/packages/cli/tests/initializeReporters.test.ts @@ -13,7 +13,9 @@ describe("initializeReporters", () => { }); afterAll(() => { - fs.rmdirSync(tmpDir, { recursive: true }); + /* eslint-disable no-console */ + console.log("Deleting tmp", tmpDir); + fs.rmSync(tmpDir, { force: true, recursive: true }); }); it("should initialize progress reporter when param is progress passed as true", () => { @@ -29,6 +31,7 @@ describe("initializeReporters", () => { expect(reporters.length).toBe(1); expect(reporters).toContainEqual(expect.any(ProgressReporter)); + reporters.forEach((r) => r.cleanup?.()); }); it("should initialize old reporter when grouped", () => { @@ -43,6 +46,7 @@ describe("initializeReporters", () => { }); expect(reporters.length).toBe(1); expect(reporters).toContainEqual(expect.any(LogReporter)); + reporters.forEach((r) => r.cleanup?.()); }); it("should initialize old reporter when verbose", () => { @@ -57,6 +61,7 @@ describe("initializeReporters", () => { }); expect(reporters.length).toBe(1); expect(reporters).toContainEqual(expect.any(LogReporter)); + reporters.forEach((r) => r.cleanup?.()); }); it("should initialize profile reporter", () => { @@ -73,6 +78,7 @@ describe("initializeReporters", () => { expect(reporters.length).toBe(2); expect(reporters).toContainEqual(expect.any(ChromeTraceEventsReporter)); + reporters.forEach((r) => r.cleanup?.()); }); it("should initialize ADO reporter when reporter arg is adoLog", () => { @@ -87,5 +93,6 @@ describe("initializeReporters", () => { }); expect(reporters.length).toBe(1); expect(reporters).toContainEqual(expect.any(AdoReporter)); + reporters.forEach((r) => r.cleanup?.()); }); }); diff --git a/packages/reporters/src/ChromeTraceEventsReporter.ts b/packages/reporters/src/ChromeTraceEventsReporter.ts index d75d42b5b..e05ed648d 100644 --- a/packages/reporters/src/ChromeTraceEventsReporter.ts +++ b/packages/reporters/src/ChromeTraceEventsReporter.ts @@ -1,5 +1,5 @@ import chalk from "chalk"; -import fs from "fs"; +import fs, { type WriteStream } from "fs"; import path from "path"; import type { Reporter } from "@lage-run/logger"; import type { SchedulerRunSummary, TargetRun } from "@lage-run/scheduler-types"; @@ -39,7 +39,7 @@ function getTimeBasedFilename(prefix: string) { } export class ChromeTraceEventsReporter implements Reporter { - logStream: Writable; + logStream: Writable | WriteStream; consoleLogStream: Writable = process.stdout; private events: TraceEventsObject = { @@ -101,4 +101,13 @@ export class ChromeTraceEventsReporter implements Reporter { ) ); } + + cleanup() { + if ("close" in this.logStream) { + this.logStream.on("error", (err: any) => { + this.consoleLogStream.write(chalk.blueBright(`\nError closing ${chalk.underline(this.outputFile)}: ${err}\n`)); + }); + this.logStream.close(); + } + } } From 0f82cfcb70a9f28e928e3d9c7eb8569f86bb0735 Mon Sep 17 00:00:00 2001 From: Dobes Vandermeer Date: Fri, 18 Apr 2025 21:32:31 -0700 Subject: [PATCH 2/3] Change files --- .../change-0203adf5-77c4-48b6-ba17-2d2094747b21.json | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 change/change-0203adf5-77c4-48b6-ba17-2d2094747b21.json diff --git a/change/change-0203adf5-77c4-48b6-ba17-2d2094747b21.json b/change/change-0203adf5-77c4-48b6-ba17-2d2094747b21.json new file mode 100644 index 000000000..26db1a0d2 --- /dev/null +++ b/change/change-0203adf5-77c4-48b6-ba17-2d2094747b21.json @@ -0,0 +1,11 @@ +{ + "changes": [ + { + "type": "none", + "comment": "Fix issue with tests failing during cleanup", + "packageName": "@lage-run/reporters", + "email": "dobes@formative.com", + "dependentChangeType": "none" + } + ] +} \ No newline at end of file From c414608bb597b20b0ba4eed914f2357c9e9b92f8 Mon Sep 17 00:00:00 2001 From: Dobes Vandermeer Date: Fri, 18 Apr 2025 21:33:00 -0700 Subject: [PATCH 3/3] Use shared reporters var and cleanup in afterEach --- .../cli/tests/initializeReporters.test.ts | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/cli/tests/initializeReporters.test.ts b/packages/cli/tests/initializeReporters.test.ts index d221ba5d9..8a225ae70 100644 --- a/packages/cli/tests/initializeReporters.test.ts +++ b/packages/cli/tests/initializeReporters.test.ts @@ -1,26 +1,30 @@ import fs from "fs"; import path from "path"; import os from "os"; -import { Logger } from "@lage-run/logger"; +import { Logger, LogStructuredData, Reporter } from "@lage-run/logger"; import { AdoReporter, ChromeTraceEventsReporter, LogReporter, ProgressReporter } from "@lage-run/reporters"; import { initializeReporters } from "../src/commands/initializeReporters.js"; describe("initializeReporters", () => { let tmpDir: string; + let reporters: Reporter[] | undefined; + + afterEach(() => { + reporters.forEach((r) => r.cleanup?.()); + reporters = undefined; + }); beforeAll(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "lage-")); }); afterAll(() => { - /* eslint-disable no-console */ - console.log("Deleting tmp", tmpDir); fs.rmSync(tmpDir, { force: true, recursive: true }); }); it("should initialize progress reporter when param is progress passed as true", () => { const logger = new Logger(); - const reporters = initializeReporters(logger, { + reporters = initializeReporters(logger, { concurrency: 1, grouped: false, logLevel: "info", @@ -31,12 +35,11 @@ describe("initializeReporters", () => { expect(reporters.length).toBe(1); expect(reporters).toContainEqual(expect.any(ProgressReporter)); - reporters.forEach((r) => r.cleanup?.()); }); it("should initialize old reporter when grouped", () => { const logger = new Logger(); - const reporters = initializeReporters(logger, { + reporters = initializeReporters(logger, { concurrency: 1, grouped: true, logLevel: "info", @@ -46,12 +49,11 @@ describe("initializeReporters", () => { }); expect(reporters.length).toBe(1); expect(reporters).toContainEqual(expect.any(LogReporter)); - reporters.forEach((r) => r.cleanup?.()); }); it("should initialize old reporter when verbose", () => { const logger = new Logger(); - const reporters = initializeReporters(logger, { + reporters = initializeReporters(logger, { concurrency: 1, grouped: false, logLevel: "info", @@ -61,12 +63,11 @@ describe("initializeReporters", () => { }); expect(reporters.length).toBe(1); expect(reporters).toContainEqual(expect.any(LogReporter)); - reporters.forEach((r) => r.cleanup?.()); }); it("should initialize profile reporter", () => { const logger = new Logger(); - const reporters = initializeReporters(logger, { + reporters = initializeReporters(logger, { concurrency: 1, grouped: false, logLevel: "info", @@ -78,12 +79,11 @@ describe("initializeReporters", () => { expect(reporters.length).toBe(2); expect(reporters).toContainEqual(expect.any(ChromeTraceEventsReporter)); - reporters.forEach((r) => r.cleanup?.()); }); it("should initialize ADO reporter when reporter arg is adoLog", () => { const logger = new Logger(); - const reporters = initializeReporters(logger, { + reporters = initializeReporters(logger, { concurrency: 1, grouped: false, logLevel: "info", @@ -93,6 +93,5 @@ describe("initializeReporters", () => { }); expect(reporters.length).toBe(1); expect(reporters).toContainEqual(expect.any(AdoReporter)); - reporters.forEach((r) => r.cleanup?.()); }); });