Skip to content

fix(testing): top-level hook outside describe no longer inflates step count #7138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fibibot wants to merge 1 commit into
base: main
Choose a base branch
from
+274 −51
Open

fix(testing): top-level hook outside describe no longer inflates step count #7138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 117 additions & 50 deletions testing/_test_suite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,27 @@ export class TestSuiteInternal<T> implements TestSuite<T> {
protected describe: DescribeDefinition<T>;
protected steps: (TestSuiteInternal<T> | ItDefinition<T>)[];
protected hasOnlyStep: boolean;
/**
* Whether this is the synthetic "global" suite created when a top-level
* `beforeAll`/`afterAll`/`beforeEach`/`afterEach` is called outside any
* `describe`. Synthetic suites are only registered with `Deno.test` if a
* top-level `it()` is also added; otherwise their hooks are inherited by
* child describes promoted to top-level `Deno.test`s.
*/
protected isSynthetic: boolean;
/**
* For a child of a synthetic global suite, this points back to the synthetic
* suite so its hooks can be invoked around the child's tests at run time.
*/
protected syntheticParent: TestSuiteInternal<T> | null;
#registeredOptions: Deno.TestDefinition | undefined;
Comment on lines +78 to 85
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small things on these fields:

  1. Both are written only in the constructor — please mark them readonly (or use a # private prefix to match #registeredOptions just below). It signals immutability and prevents accidental mutation.
  2. syntheticParent reads slightly off because the parent is what's synthetic, not the field itself. Something like parentSynthetic or globalHookSuite would be clearer. Taste call.

Also worth noting in the syntheticParent JSDoc that this is an asymmetric linkage — normal parents are reached via describe.suite, only synthetic globals use this back-reference. Future readers will wonder why two pointers exist.


constructor(describe: DescribeDefinition<T>) {
constructor(describe: DescribeDefinition<T>, isSynthetic = false) {
this.describe = describe;
this.steps = [];
this.hasOnlyStep = false;
this.isSynthetic = isSynthetic;
this.syntheticParent = null;

const { suite } = describe;
if (suite && !TestSuiteInternal.suites.has(suite.symbol)) {
Expand Down Expand Up @@ -138,41 +153,82 @@ export class TestSuiteInternal<T> implements TestSuite<T> {
}
}

if (testSuite) {
if (testSuite && testSuite.isSynthetic) {
// Promote: a child describe of the synthetic global is registered as its
// own top-level Deno.test rather than as a step of the global suite. The
// child inherits the global's hooks at run time via syntheticParent.
this.syntheticParent = testSuite;
this.registerAsDenoTest();
Comment on lines +156 to +161
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it.only propagation worth double-checking here. addingOnlyStep walks parents via suite.describe.suite, which is not set for a promoted child (its synthetic linkage lives on syntheticParent). Two cases:

  • it.only() inside the promoted describe → works, because hasOnlyStep is set during fn() before registerAsDenoTest() reads it.
  • Top-level it.only() plus a separate describe(...) → the synthetic becomes a real Deno.test marked only, but the promoted describe is neither only nor ignore. Pre-PR, the describe was a step of the synthetic and got filtered out by only; post-PR its tests will run alongside the only test.

If that's an accepted behavior change, please add a regression test that pins it down. If it's a regression, the fix is probably to propagate only/ignore from syntheticParent into the promoted child at registerAsDenoTest() time.

} else if (testSuite) {
TestSuiteInternal.addStep(testSuite, this);
} else {
const {
name,
ignore,
permissions,
sanitizeExit = globalSanitizersState.sanitizeExit,
sanitizeOps = globalSanitizersState.sanitizeOps,
sanitizeResources = globalSanitizersState.sanitizeResources,
} = describe;
let { only } = describe;
if (!ignore && this.hasOnlyStep) {
only = true;
}
const options: Deno.TestDefinition = {
name,
fn: async (t) => {
TestSuiteInternal.runningCount++;
try {
const context = {} as T;
const { beforeAll } = this.describe;
} else if (!this.isSynthetic) {
this.registerAsDenoTest();
}
// Synthetic suites without a parent are not registered eagerly. They are
// registered lazily by `addStep` when a top-level `it()` is added.
}

/** Builds the Deno.test options for this suite and registers them. */
protected registerAsDenoTest() {
if (this.#registeredOptions) return;
const {
name,
ignore,
permissions,
sanitizeExit = globalSanitizersState.sanitizeExit,
sanitizeOps = globalSanitizersState.sanitizeOps,
sanitizeResources = globalSanitizersState.sanitizeResources,
} = this.describe;
let { only } = this.describe;
if (!ignore && this.hasOnlyStep) {
only = true;
}
const options: Deno.TestDefinition = {
name,
fn: async (t) => {
TestSuiteInternal.runningCount++;
try {
const context = {} as T;
const parent = this.syntheticParent;
if (parent) {
const { beforeAll } = parent.describe;
if (typeof beforeAll === "function") {
await beforeAll.call(context);
} else if (beforeAll) {
for (const hook of beforeAll) {
await hook.call(context);
}
}
try {
TestSuiteInternal.active.push(this.symbol);
await TestSuiteInternal.run(this, context, t);
} finally {
}
const { beforeAll } = this.describe;
if (typeof beforeAll === "function") {
await beforeAll.call(context);
} else if (beforeAll) {
for (const hook of beforeAll) {
await hook.call(context);
}
}
try {
if (parent) {
TestSuiteInternal.active.push(parent.symbol);
}
TestSuiteInternal.active.push(this.symbol);
await TestSuiteInternal.run(this, context, t);
} finally {
TestSuiteInternal.active.pop();
if (parent) {
TestSuiteInternal.active.pop();
const { afterAll } = this.describe;
}
const { afterAll } = this.describe;
if (typeof afterAll === "function") {
await afterAll.call(context);
} else if (afterAll) {
for (const hook of afterAll) {
await hook.call(context);
}
Comment on lines +192 to +228
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the typeof fn === "function" / for (const hook of …) pattern now appears four times in this one function (parent-before, this-before, this-after, parent-after). The existing run/runTest use the same pattern, so this is at least consistent — but a tiny runHooks(hooks, context) helper would tighten things up a lot. Optional.

}
if (parent) {
const { afterAll } = parent.describe;
if (typeof afterAll === "function") {
await afterAll.call(context);
} else if (afterAll) {
Expand All @@ -181,31 +237,31 @@ export class TestSuiteInternal<T> implements TestSuite<T> {
}
}
}
} finally {
TestSuiteInternal.runningCount--;
}
},
};
if (ignore !== undefined) {
options.ignore = ignore;
}
if (only !== undefined) {
options.only = only;
}
if (permissions !== undefined) {
options.permissions = permissions;
}
if (sanitizeExit !== undefined) {
options.sanitizeExit = sanitizeExit;
}
if (sanitizeOps !== undefined) {
options.sanitizeOps = sanitizeOps;
}
if (sanitizeResources !== undefined) {
options.sanitizeResources = sanitizeResources;
}
this.#registeredOptions = TestSuiteInternal.registerTest(options);
} finally {
TestSuiteInternal.runningCount--;
}
},
};
if (ignore !== undefined) {
options.ignore = ignore;
}
if (only !== undefined) {
options.only = only;
}
if (permissions !== undefined) {
options.permissions = permissions;
}
if (sanitizeExit !== undefined) {
options.sanitizeExit = sanitizeExit;
}
if (sanitizeOps !== undefined) {
options.sanitizeOps = sanitizeOps;
}
if (sanitizeResources !== undefined) {
options.sanitizeResources = sanitizeResources;
}
this.#registeredOptions = TestSuiteInternal.registerTest(options);
}

/** Stores how many test suites are executing. */
Expand Down Expand Up @@ -289,6 +345,17 @@ export class TestSuiteInternal<T> implements TestSuite<T> {
suite: TestSuiteInternal<T>,
step: TestSuiteInternal<T> | ItDefinition<T>,
) {
// When adding a top-level `it()` to the synthetic global suite, the global
// needs to become a real `Deno.test` so the test has a place to run.
// Child `describe`s are promoted at construction time and never reach
// `addStep` with the synthetic suite as their parent.
if (
suite.isSynthetic && !suite.#registeredOptions &&
!(step instanceof TestSuiteInternal)
) {
suite.registerAsDenoTest();
Comment on lines +348 to +356
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the mixed case (top-level hook + top-level it() + nested describe) materializes: the synthetic gets registered here as a real Deno.test, while the sibling describes have already been promoted. End result is that the global beforeAll fires once per top-level Deno.test — twice or more across the file — rather than strictly once. The PR body calls this out as accepted, and I think that's fine, but please:

  1. Add a regression test that asserts the new behavior (two Deno.test calls, global beforeAll runs in each). Without one, a future refactor could silently change it back.
  2. Consider a short JSDoc note near addHook in bdd.ts (or in the testing/bdd.ts module docs) so users mixing styles aren't surprised.

}

if (!suite.hasOnlyStep) {
if (step instanceof TestSuiteInternal) {
if (step.hasOnlyStep || step.describe.only) {
Expand Down
2 changes: 1 addition & 1 deletion testing/bdd.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ function addHook<T>(
TestSuiteInternal.current = new TestSuiteInternal({
name: "global",
[name]: fn,
});
}, true);
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a one-line comment here explaining why true — e.g. // Mark as synthetic so it isn't eagerly registered as a Deno.test; see TestSuiteInternal#isSynthetic. The boolean literal at a call site is otherwise opaque.

} else {
TestSuiteInternal.setHook(TestSuiteInternal.current!, name, fn);
}
Expand Down
156 changes: 156 additions & 0 deletions testing/bdd_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,162 @@ Deno.test("beforeAll(), afterAll(), beforeEach() and afterEach()", async () => {
assertSpyCalls(afterEachFn, 2);
});

Deno.test(
"top-level beforeAll() with only nested describe() does not add an extra step",
async () => {
using test = stub(Deno, "test");
const fns = [spy(), spy()] as const;
const { beforeAllFn, afterAllFn, beforeEachFn, afterEachFn } = hookFns();

const context = new TestContext("the describe");
try {
beforeAll(beforeAllFn);
afterAll(afterAllFn);
beforeEach(beforeEachFn);
afterEach(afterEachFn);

describe("the describe", () => {
it({ name: "test 1", fn: fns[0] });
it({ name: "test 2", fn: fns[1] });
});

// Only one Deno.test should be registered, named after the user's
// describe — not a synthetic "global" wrapper. Without the fix this
// is called twice (once for "global", once as a step), inflating the
// step count reported by `deno test`.
assertSpyCalls(test, 1);
const options = test.calls[0]?.args[0] as Deno.TestDefinition;
assertEquals(Object.keys(options).sort(), ["fn", "name"]);
assertEquals(options.name, "the describe");

const result = options.fn(context);
assertStrictEquals(Promise.resolve(result), result);
assertEquals(await result, undefined);
// Only the two user tests should appear as steps; no extra wrapping step.
assertSpyCalls(context.spies.step, 2);
} finally {
TestSuiteInternal.reset();
}

assertSpyCalls(fns[0], 1);
assertSpyCalls(fns[1], 1);

// Top-level hooks still run around the nested tests.
assertSpyCalls(beforeAllFn, 1);
assertSpyCalls(afterAllFn, 1);
assertSpyCalls(beforeEachFn, 2);
assertSpyCalls(afterEachFn, 2);
},
);

Deno.test(
"top-level beforeAll() with only nested describe() still wires up hooks when describe has its own hooks",
async () => {
using test = stub(Deno, "test");
const fns = [spy(), spy()] as const;
const globalBeforeAll = spy();
const globalAfterAll = spy();
const globalBeforeEach = spy();
const globalAfterEach = spy();
const localBeforeAll = spy();
const localAfterAll = spy();
const localBeforeEach = spy();
const localAfterEach = spy();

const order: string[] = [];
const trackOrder = (label: string) => () => {
order.push(label);
};

const context = new TestContext("d");
try {
beforeAll(spy(trackOrder("globalBeforeAll")));
beforeAll(globalBeforeAll);
afterAll(globalAfterAll);
afterAll(spy(trackOrder("globalAfterAll")));
beforeEach(spy(trackOrder("globalBeforeEach")));
beforeEach(globalBeforeEach);
afterEach(globalAfterEach);
afterEach(spy(trackOrder("globalAfterEach")));

describe("d", () => {
beforeAll(spy(trackOrder("localBeforeAll")));
beforeAll(localBeforeAll);
afterAll(localAfterAll);
afterAll(spy(trackOrder("localAfterAll")));
beforeEach(spy(trackOrder("localBeforeEach")));
beforeEach(localBeforeEach);
afterEach(localAfterEach);
afterEach(spy(trackOrder("localAfterEach")));

it({ name: "t1", fn: fns[0] });
it({ name: "t2", fn: fns[1] });
});

assertSpyCalls(test, 1);
const options = test.calls[0]?.args[0] as Deno.TestDefinition;
assertEquals(options.name, "d");

await options.fn(context);
} finally {
TestSuiteInternal.reset();
}

// Global hooks wrap local hooks around the tests.
assertEquals(order, [
"globalBeforeAll",
"localBeforeAll",
"globalBeforeEach",
"localBeforeEach",
"localAfterEach",
"globalAfterEach",
"globalBeforeEach",
"localBeforeEach",
"localAfterEach",
"globalAfterEach",
"localAfterAll",
"globalAfterAll",
]);

assertSpyCalls(globalBeforeAll, 1);
assertSpyCalls(globalAfterAll, 1);
assertSpyCalls(globalBeforeEach, 2);
assertSpyCalls(globalAfterEach, 2);
assertSpyCalls(localBeforeAll, 1);
assertSpyCalls(localAfterAll, 1);
assertSpyCalls(localBeforeEach, 2);
assertSpyCalls(localAfterEach, 2);
assertSpyCalls(fns[0], 1);
assertSpyCalls(fns[1], 1);
},
);

Deno.test(
"top-level beforeAll() with multiple nested describes registers each describe as its own Deno.test",
() => {
using test = stub(Deno, "test");
const beforeAllFn = spy();
try {
beforeAll(beforeAllFn);

describe("d1", () => {
it({ name: "t1", fn: () => {} });
});
describe("d2", () => {
it({ name: "t2", fn: () => {} });
});

assertSpyCalls(test, 2);
const first = test.calls[0]?.args[0] as Deno.TestDefinition;
const second = test.calls[1]?.args[0] as Deno.TestDefinition;
assertEquals(first.name, "d1");
assertEquals(second.name, "d2");
} finally {
TestSuiteInternal.reset();
}
},
);

Deno.test("beforeAll() with it.only() propagates only to Deno.test", () => {
using test = stub(Deno, "test");
try {
Expand Down
Loading