Skip to content

improvement(test-version-utils): Share TestObjectProvider creation hooks #24662

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
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

improvement(test-version-utils): Share TestObjectProvider creation hooks #24662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
42bcd0c
Fix various e2e test memory leaks
May 16, 2025
97969c0
include missed change
May 16, 2025
0ea0223
fix lint
May 16, 2025
c00f90c
Fix timeoutUtils functionality
May 17, 2025
9f76cf0
Fix hook ordering and remove some stray changes
May 19, 2025
7816514
Share common test object provider initialization hooks between descri…
May 19, 2025
c29a923
merge main
May 20, 2025
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
220 changes: 122 additions & 98 deletions packages/test/test-version-utils/src/describeCompat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,103 @@ import { getRequestedVersion } from "./versionUtils.js";
// See doc comment on mochaGlobalSetup.
await mochaGlobalSetup();

/**
* Alias for `() => T` to make it clear that the return value may change over the course of test execution.
*/
type ReadonlyMutableState<T> = () => T;

export function createTestObjectProviderLifecycleHooks(
providerFactory: () => Promise<ITestObjectProvider>,
getResetAfterEach: ReadonlyMutableState<boolean>,
initializeTimeout?: number,
): ReadonlyMutableState<ITestObjectProvider> {
let currentProvider: ITestObjectProvider | undefined;
const getProvider = () => {
if (currentProvider === undefined) {
throw new Error("Provider is only accessible during mocha hooks or test execution.");
}
return currentProvider;
};

before("Create TestObjectProvider", async function () {
if (initializeTimeout !== undefined) {
const timeout = this.timeout();
// timeout 0 indicates no timeout and explicitly changing it can interrupt debugging flows.
this.timeout(timeout === 0 ? 0 : initializeTimeout);
}
try {
currentProvider = await providerFactory();
} catch (error) {
const logger = createChildLogger({
logger: getTestLogger?.(),
namespace: "DescribeCompatSetup",
});
logger.sendErrorEvent(
{
eventName: "TestObjectProviderLoadFailed",
driverType: driver,
},
error,
);
throw error;
}

Object.defineProperty(this, "__fluidTestProvider", {
get: () => currentProvider,
configurable: true,
});
});

afterEach("Verify container telemetry", function (done: Mocha.Done) {
const provider = getProvider();
const logErrors = getUnexpectedLogErrorException(provider.tracker);
// if the test failed for another reason
// then we don't need to check errors
// and fail the after each as well.
// This also avoids failing tests that are skipped from inside the test body, which is
// a pattern we use to only run tests on certain drivers.
if (this.currentTest?.state === "passed") {
done(logErrors);
} else {
done();
}
});

afterEach("Reset TestObjectProvider", () => {
if (getResetAfterEach()) {
const provider = getProvider();
provider.reset();
}
});

// Mocha contexts are long-lived, and leaking the testObjectProvider on them severely eats into
// memory over the course of our e2e tests. This is especially bad for local server, where the
// server ends up retaining direct references to containers. This hook resolves that issue by explicitly
// removing retainers for the test object provider from the context.
// A good way to test memory impact of changes here is by doing one of:
// - Put an existing e2e test's `it` block in a loop to create many copies of it and run only this test
// - Put a single test in a `describeCompat` block and put the `describeCompat` block in a loop
// then taking heap snapshots over the course of various runs.
// Because of things like the summarizer process, containers may not be GC'd as soon as tests are done executing,
// but you should see the total number of retained containers as well as server objects stabilize over time rather than grow.
// Heap snapshots for a large number of tests within a single suite help detect bugs with leaking objects while a suite executes,
// which is problematic for suites that run a large number of test cases (usually combintorially generated).
// Heap snapshots for a large number of suites help detect bugs with leaking objects across suites,
// which is problematic for issues that tend to get hit "later in the overall test run".
after("Cleanup TestObjectProvider", function () {
const provider = getProvider();
provider.driver.dispose?.();
currentProvider = undefined;
Object.defineProperty(this, "__fluidTestProvider", {
get: () => {
throw new Error("Attempted to use __fluidTestProvider after test suite disposed.");
},
});
});

return getProvider;
}

/*
* Mocha Utils for test to generate the compat variants.
*/
Expand Down Expand Up @@ -70,54 +167,36 @@ function createCompatSuite(
continue;
}
describe(config.name, function () {
let provider: ITestObjectProvider | undefined;
let resetAfterEach: boolean;
const apis: CompatApis = getVersionedApis(config);
const providerFactory = async (): Promise<ITestObjectProvider> =>
config.kind === CompatKind.CrossClient
? // Awaiting the return gives a clearer stack trace.
// eslint-disable-next-line @typescript-eslint/return-await
await getCompatVersionedTestObjectProviderFromApis(apis, {
type: driver,
config: {
r11s: { r11sEndpointName },
odsp: { tenantIndex, odspEndpointName },
},
})
: // eslint-disable-next-line @typescript-eslint/return-await
await getVersionedTestObjectProviderFromApis(apis, {
type: driver,
config: {
r11s: { r11sEndpointName },
odsp: { tenantIndex, odspEndpointName },
},
});

before("Create TestObjectProvider", async function () {
try {
provider =
config.kind === CompatKind.CrossClient
? await getCompatVersionedTestObjectProviderFromApis(apis, {
type: driver,
config: {
r11s: { r11sEndpointName },
odsp: { tenantIndex, odspEndpointName },
},
})
: await getVersionedTestObjectProviderFromApis(apis, {
type: driver,
config: {
r11s: { r11sEndpointName },
odsp: { tenantIndex, odspEndpointName },
},
});
} catch (error) {
const logger = createChildLogger({
logger: getTestLogger?.(),
namespace: "DescribeCompatSetup",
});
logger.sendErrorEvent(
{
eventName: "TestObjectProviderLoadFailed",
driverType: driver,
},
error,
);
throw error;
}

Object.defineProperty(this, "__fluidTestProvider", {
get: () => provider,
configurable: true,
});
});
let resetAfterEach: boolean = true;
const getProvider = createTestObjectProviderLifecycleHooks(
providerFactory,
() => resetAfterEach,
);

tests.bind(this)((options?: ITestObjectProviderOptions) => {
resetAfterEach = options?.resetAfterEach ?? true;
if (provider === undefined) {
throw new Error("Expected provider to be set up by before hook");
}
const provider = getProvider();
if (options?.syncSummarizer === true) {
provider.resetLoaderContainerTracker(true /* syncSummarizerClients */);
}
Expand All @@ -126,61 +205,6 @@ function createCompatSuite(
}
return provider;
}, apis);

afterEach("Verify container telemetry", function (done: Mocha.Done) {
if (provider === undefined) {
throw new Error("Expected provider to be set up by before hook");
}
const logErrors = getUnexpectedLogErrorException(provider.tracker);
// if the test failed for another reason
// then we don't need to check errors
// and fail the after each as well.
// This also avoids failing tests that are skipped from inside the test body, which is
// a pattern we use to only run tests on certain drivers.
if (this.currentTest?.state === "passed") {
done(logErrors);
} else {
done();
}
});

afterEach("Reset TestObjectProvider", () => {
if (provider === undefined) {
throw new Error("Expected provider to be set up by before hook");
}
if (resetAfterEach) {
provider.reset();
}
});

// Mocha contexts are long-lived, and leaking the testObjectProvider on them severely eats into
// memory over the course of our e2e tests. This is especially bad for local server, where the
// server ends up retaining direct references to containers. This hook resolves that issue by explicitly
// removing retainers for the test object provider from the context.
// A good way to test memory impact of changes here is by doing one of:
// - Put an existing e2e test's `it` block in a loop to create many copies of it and run only this test
// - Put a single test in a `describeCompat` block and put the `describeCompat` block in a loop
// then taking heap snapshots over the course of various runs.
// Because of things like the summarizer process, containers may not be GC'd as soon as tests are done executing,
// but you should see the total number of retained containers as well as server objects stabilize over time rather than grow.
// Heap snapshots for a large number of tests within a single suite help detect bugs with leaking objects while a suite executes,
// which is problematic for suites that run a large number of test cases (usually combintorially generated).
// Heap snapshots for a large number of suites help detect bugs with leaking objects across suites,
// which is problematic for issues that tend to get hit "later in the overall test run".
after("Cleanup TestObjectProvider", function () {
if (provider === undefined) {
throw new Error("Expected provider to be set up by before hook");
}
provider.driver.dispose?.();
provider = undefined;
Object.defineProperty(this, "__fluidTestProvider", {
get: () => {
throw new Error(
"Attempted to use __fluidTestProvider after test suite disposed.",
);
},
});
});
});
}
};
Expand Down
72 changes: 22 additions & 50 deletions packages/test/test-version-utils/src/describeE2eDocs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,7 @@
import fs from "fs";

import { TestDriverTypes } from "@fluid-internal/test-driver-definitions";
import { createChildLogger } from "@fluidframework/telemetry-utils/internal";
import {
getUnexpectedLogErrorException,
ITestObjectProvider,
TestObjectProvider,
} from "@fluidframework/test-utils/internal";
import { ITestObjectProvider } from "@fluidframework/test-utils/internal";

import { testBaseVersion } from "./baseVersion.js";
import { configList } from "./compatConfig.js";
Expand All @@ -23,7 +18,10 @@ import {
tenantIndex,
} from "./compatOptions.js";
import { getVersionedTestObjectProviderFromApis } from "./compatUtils.js";
import { ITestObjectProviderOptions } from "./describeCompat.js";
import {
createTestObjectProviderLifecycleHooks,
ITestObjectProviderOptions,
} from "./describeCompat.js";
import {
getDataRuntimeApi,
getLoaderApi,
Expand Down Expand Up @@ -351,8 +349,6 @@ function createE2EDocCompatSuite(
for (const doctype of docTypes) {
const name = `${title} - ${doctype.testTitle}`;
describe(name, function () {
let provider: TestObjectProvider;
let resetAfterEach: boolean;
const dataRuntimeApi = getDataRuntimeApi(
getRequestedVersion(testBaseVersion(config.dataRuntime), config.dataRuntime),
);
Expand All @@ -372,36 +368,27 @@ function createE2EDocCompatSuite(
getRequestedVersion(testBaseVersion(config.loader), config.loader),
),
};
const providerFactory = async (): Promise<ITestObjectProvider> =>
// Awaiting the return gives a clearer stack trace.
// eslint-disable-next-line @typescript-eslint/return-await
await getVersionedTestObjectProviderFromApis(apis, {
type: driver,
config: {
r11s: { r11sEndpointName },
odsp: { tenantIndex, odspEndpointName },
},
});

let resetAfterEach: boolean = true;
const getProvider = createTestObjectProviderLifecycleHooks(
providerFactory,
() => resetAfterEach,
);

before(async function () {
try {
provider = await getVersionedTestObjectProviderFromApis(apis, {
type: driver,
config: {
r11s: { r11sEndpointName },
odsp: { tenantIndex, odspEndpointName },
},
});
} catch (error) {
const logger = createChildLogger({
logger: getTestLogger?.(),
namespace: "DescribeE2EDocs",
});
logger.sendErrorEvent(
{
eventName: "TestObjectProviderLoadFailed",
driverType: driver,
},
error,
);
throw error;
}

Object.defineProperty(this, "__fluidTestProvider", { get: () => provider });
});
tests.bind(this)(
(options?: ITestObjectProviderOptions) => {
resetAfterEach = options?.resetAfterEach ?? true;
const provider = getProvider();
if (options?.syncSummarizer === true) {
provider.resetLoaderContainerTracker(true /* syncSummarizerClients */);
}
Expand All @@ -411,21 +398,6 @@ function createE2EDocCompatSuite(
return doctype;
},
);

afterEach(function (done: Mocha.Done) {
// if the test failed for another reason
// then we don't need to check errors
// and fail the after each as well
if (this.currentTest?.state === "passed") {
const logErrors = getUnexpectedLogErrorException(provider.tracker);
done(logErrors);
} else {
done();
}
if (resetAfterEach) {
provider.reset();
}
});
});
}
}
Expand Down
Loading
Loading