Skip to content

Commit bf869f5

Browse files
markfieldsmarkfields
authored
fix(benchmark tests): Ensure "benchmark end" event is emitted even if an error is thrown by the runner (#26018)
One of my benchmark tests fails intermittently with the error `runtime benchmarks Non-Compat completed with status 'failed' without reporting any data.` This indicates that the "benchmark end" event was never emitted. I added a try-catch to see if that fixes it, along with a suggested refactor to share code.
1 parent b084ac5 commit bf869f5

File tree

4 files changed

+152
-79
lines changed

4 files changed

+152
-79
lines changed

‎tools/benchmark/src/ResultUtilities.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
/*!
2+
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
3+
* Licensed under the MIT License.
4+
*/
5+
6+
import type { BenchmarkResult, BenchmarkError, BenchmarkData, CustomData } from "./ResultTypes";
7+
import { timer } from "./timer";
8+
9+
/**
10+
* Wraps a function that returns CustomData, measuring its execution time
11+
* and capturing either its result or exception.
12+
* Returns a callback suitable for passing to emitResultsMocha.
13+
* This is a generic utility that is neither mocha-specific nor time benchmark-specific.
14+
*/
15+
export function captureResults(
16+
f: () => CustomData | Promise<CustomData>,
17+
): () => Promise<{ result: BenchmarkResult; exception?: Error }> {
18+
return async () => {
19+
const startTime = timer.now();
20+
21+
let customData: CustomData;
22+
try {
23+
customData = await f();
24+
} catch (error) {
25+
const benchmarkError: BenchmarkError = { error: (error as Error).message };
26+
return { result: benchmarkError, exception: error as Error };
27+
}
28+
29+
const elapsedSeconds = timer.toSeconds(startTime, timer.now());
30+
31+
const result: BenchmarkData = {
32+
elapsedSeconds,
33+
customData,
34+
};
35+
36+
return { result };
37+
};
38+
}

‎tools/benchmark/src/mocha/customOutputRunner.ts‎

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@
66
import type { Test } from "mocha";
77

88
import type { BenchmarkDescription, MochaExclusiveOptions, Titled } from "../Configuration";
9-
import type { BenchmarkData, BenchmarkError, CustomData } from "../ResultTypes";
9+
import type { CustomData } from "../ResultTypes";
10+
import { captureResults } from "../ResultUtilities";
1011
import { prettyNumber } from "../RunnerUtilities";
11-
import { timer } from "../timer";
12+
13+
import { emitResultsMocha } from "./runnerUtilities";
1214

1315
/**
1416
* Options to configure a benchmark that reports custom measurements.
@@ -48,26 +50,14 @@ export function benchmarkCustom(options: CustomBenchmarkOptions): Test {
4850
},
4951
};
5052

51-
const startTime = timer.now();
52-
53-
try {
54-
await options.run(reporter);
55-
} catch (error) {
56-
const benchmarkError: BenchmarkError = { error: (error as Error).message };
57-
58-
test.emit("benchmark end", benchmarkError);
59-
60-
throw error;
61-
}
62-
63-
const elapsedSeconds = timer.toSeconds(startTime, timer.now());
64-
65-
const results: BenchmarkData = {
66-
elapsedSeconds,
67-
customData,
68-
};
69-
70-
test.emit("benchmark end", results);
53+
// Emits the "benchmark end" event with the result
54+
await emitResultsMocha(
55+
captureResults(async () => {
56+
await options.run(reporter);
57+
return customData;
58+
}),
59+
test,
60+
);
7161
});
7262
return test;
7363
}

‎tools/benchmark/src/mocha/runner.ts‎

Lines changed: 79 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ import type { BenchmarkResult } from "../ResultTypes";
1616
import { fail } from "../assert.js";
1717
import { Phase, runBenchmark } from "../runBenchmark";
1818

19+
import { emitResultsMocha } from "./runnerUtilities";
20+
1921
/**
2022
* This is wrapper for Mocha's it function that runs a performance benchmark.
2123
*
@@ -64,72 +66,92 @@ export function supportParentProcess<
6466
const itFunction = args.only === true ? it.only : it;
6567
const test = itFunction(args.title, async () => {
6668
if (isParentProcess) {
67-
// Instead of running the benchmark in this process, create a new process.
68-
// See {@link isParentProcess} for why.
69-
// Launch new process, with:
70-
// - mocha filter to run only this test.
71-
// - --parentProcess flag removed.
72-
// - --childProcess flag added (so data will be returned via stdout as json)
73-
74-
// Pull the command (Node.js most likely) out of the first argument since spawnSync takes it separately.
75-
const command = process.argv0 ?? fail("there must be a command");
76-
77-
// We expect all node-specific flags to be present in execArgv so they can be passed to the child process.
78-
// At some point mocha was processing the expose-gc flag itself and not passing it here, unless explicitly
79-
// put in mocha's --node-option flag.
80-
const childArgs = [...process.execArgv, ...process.argv.slice(1)];
81-
const processFlagIndex = childArgs.indexOf("--parentProcess");
82-
childArgs[processFlagIndex] = "--childProcess";
83-
84-
// Remove arguments for any existing test filters.
85-
for (const flag of ["--grep", "--fgrep"]) {
86-
const flagIndex = childArgs.indexOf(flag);
87-
if (flagIndex > 0) {
88-
// Remove the flag, and the argument after it (all these flags take one argument)
89-
childArgs.splice(flagIndex, 2);
90-
}
91-
}
69+
await emitResultsMocha(async () => {
70+
try {
71+
// Instead of running the benchmark in this process, create a new process.
72+
// See {@link isParentProcess} for why.
73+
// Launch new process, with:
74+
// - mocha filter to run only this test.
75+
// - --parentProcess flag removed.
76+
// - --childProcess flag added (so data will be returned via stdout as json)
9277

93-
// Add test filter so child process only run the current test.
94-
childArgs.push("--fgrep", test.fullTitle());
95-
96-
// Remove arguments for debugging if they're present; in order to debug child processes we need
97-
// to specify a new debugger port for each, or they'll fail to start. Doable, but leaving it out
98-
// of scope for now.
99-
let inspectArgIndex: number = -1;
100-
while (
101-
(inspectArgIndex = childArgs.findIndex((x) => x.match(/^(--inspect|--debug).*/))) >=
102-
0
103-
) {
104-
childArgs.splice(inspectArgIndex, 1);
105-
}
78+
// Pull the command (Node.js most likely) out of the first argument since spawnSync takes it separately.
79+
const command = process.argv0 ?? fail("there must be a command");
10680

107-
// Do this import only if isParentProcess to enable running in the web as long as isParentProcess is false.
108-
const childProcess = await import("node:child_process");
109-
const result = childProcess.spawnSync(command, childArgs, { encoding: "utf8" });
81+
// We expect all node-specific flags to be present in execArgv so they can be passed to the child process.
82+
// At some point mocha was processing the expose-gc flag itself and not passing it here, unless explicitly
83+
// put in mocha's --node-option flag.
84+
const childArgs = [...process.execArgv, ...process.argv.slice(1)];
85+
const processFlagIndex = childArgs.indexOf("--parentProcess");
86+
childArgs[processFlagIndex] = "--childProcess";
11087

111-
if (result.error) {
112-
fail(`Child process reported an error: ${result.error.message}`);
113-
}
88+
// Remove arguments for any existing test filters.
89+
for (const flag of ["--grep", "--fgrep"]) {
90+
const flagIndex = childArgs.indexOf(flag);
91+
if (flagIndex > 0) {
92+
// Remove the flag, and the argument after it (all these flags take one argument)
93+
childArgs.splice(flagIndex, 2);
94+
}
95+
}
11496

115-
if (result.stderr !== "") {
116-
fail(`Child process logged errors: ${result.stderr}`);
117-
}
97+
// Add test filter so child process only run the current test.
98+
childArgs.push("--fgrep", test.fullTitle());
11899

119-
// Find the json blob in the child's output.
120-
const output =
121-
result.stdout.split("\n").find((s) => s.startsWith("{")) ??
122-
fail(`child process must output a json blob. Got:\n${result.stdout}`);
100+
// Remove arguments for debugging if they're present; in order to debug child processes we need
101+
// to specify a new debugger port for each, or they'll fail to start. Doable, but leaving it out
102+
// of scope for now.
103+
let inspectArgIndex: number = -1;
104+
while (
105+
(inspectArgIndex = childArgs.findIndex((x) =>
106+
x.match(/^(--inspect|--debug).*/),
107+
)) >= 0
108+
) {
109+
childArgs.splice(inspectArgIndex, 1);
110+
}
123111

124-
test.emit("benchmark end", JSON.parse(output));
112+
// Do this import only if isParentProcess to enable running in the web as long as isParentProcess is false.
113+
const childProcess = await import("node:child_process");
114+
const result = childProcess.spawnSync(command, childArgs, { encoding: "utf8" });
115+
116+
if (result.error) {
117+
fail(`Child process reported an error: ${result.error.message}`);
118+
}
119+
120+
if (result.stderr !== "") {
121+
fail(`Child process logged errors: ${result.stderr}`);
122+
}
123+
124+
// Find the json blob in the child's output.
125+
const output =
126+
result.stdout.split("\n").find((s) => s.startsWith("{")) ??
127+
fail(`child process must output a json blob. Got:\n${result.stdout}`);
128+
129+
return { result: JSON.parse(output) as BenchmarkResult };
130+
} catch (error) {
131+
return {
132+
result: { error: (error as Error).message },
133+
exception: error as Error,
134+
};
135+
}
136+
}, test);
125137
return;
126138
}
127139

128-
const stats = await args.run();
129-
// Create and run a benchmark if we are in perfMode, else run the passed in function normally
130-
if (isInPerformanceTestingMode) {
131-
test.emit("benchmark end", stats);
132-
}
140+
// Only emit results in perfMode
141+
await (isInPerformanceTestingMode
142+
? emitResultsMocha(
143+
async () =>
144+
args.run().then(
145+
(result) => ({ result }),
146+
(error) => ({
147+
result: { error: (error as Error).message },
148+
exception: error as Error,
149+
}),
150+
),
151+
test,
152+
)
153+
: // In non-perf mode, just run the function without emitting
154+
args.run());
133155
});
134156
return test;
135157
}

‎tools/benchmark/src/mocha/runnerUtilities.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/*!
2+
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
3+
* Licensed under the MIT License.
4+
*/
5+
6+
import type { Test } from "mocha";
7+
8+
import type { BenchmarkResult } from "../ResultTypes";
9+
10+
/**
11+
* Executes a function, emits the results to a mocha test, and throws any exception.
12+
* This handles mocha-specific result emission.
13+
*/
14+
export async function emitResultsMocha(
15+
f: () => Promise<{ result: BenchmarkResult; exception?: Error }>,
16+
test: Test,
17+
): Promise<void> {
18+
const { exception, result } = await f();
19+
test.emit("benchmark end", result);
20+
if (exception !== undefined) {
21+
throw exception;
22+
}
23+
}

0 commit comments

Comments
 (0)