Fix and reorganize stashed ops that wait for a summary #23542

dannimad · 2025-01-14T02:01:32Z

Some tests among stashedOps.spec.ts need to wait for a summary in order to fall into its test scenario. There was some missing configuration on them:

ensureSynchonized should be called before summarizing to ensure that it summaries all changes up to that point.
When created the provider via getTestObjectProvider, pass in option { syncSummarizer: true } so that ensureSynchonized will sync summarizer clients.
explicitly load summarizers from the snapshot generated by previous summarizers by specifying the version of that snapshot

Besides those changes, I made some reorganization to these tests since added config is necessary only for tests that wait for a summary so I separated them into a different file and move some functions to a utils file well.

Some of these tests are currently failing in ODSP and so I'll head up OCE in case they keep failing after this PR is merged. If this approach work, I'll use it for refresh snapshot tests that are currently failing for what I suspect the same reason.

Copilot

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

packages/test/test-end-to-end-tests/src/test/offline/offlineTestsUtils.ts:43

[nitpick] The magic number 30 should be defined as a constant or passed as a parameter for better readability and maintainability.

const lots = 30;

packages/test/test-end-to-end-tests/src/test/offline/containerDirtyFlag.spec.ts:31

Ensure that map2 is properly initialized before using it. Add a check to verify that map2 is not null or undefined.

const map2 = await dataStore2.getSharedObject<ISharedMap>(mapId);

Copilot · 2025-01-14T02:02:35Z

packages/test/test-end-to-end-tests/src/test/offline/offlineTestsUtils.ts

+export async function loadOffline(
+	testContainerConfig: ITestContainerConfig,
+	testObjectProvider: ITestObjectProvider,
+	request: IRequest,


The request parameter should be validated to ensure it is not null or undefined before being used.

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts

agarwal-navin · 2025-01-16T23:41:49Z

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts

+	(getTestObjectProvider, apis) => {
+		const mapId = "map";
+		const { SharedMap } = apis.dds;
+		const registry: ChannelFactoryRegistry = [[mapId, SharedMap.getFactory()]];


Do you really need a map? It would be simpler and less code to use the default root DDS that the test data object exposes.

Map is not really needed but I'm trying to keep tests as close as we have them as stashedOps.spec.ts, so it's more just for consistency purposes.

agarwal-navin · 2025-01-16T23:42:18Z

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts

+		let map1: ISharedMap;
+		let dataStore1: ITestFluidObject;
+		const testContainerConfig: ITestContainerConfig = {
+			fluidDataObjectType: DataObjectFactoryType.Test,


Do you need to pass this? The default test object created by the test object provider should be sufficient right?

Same as map, just trying to keep it consistent with other offline tests.

agarwal-navin · 2025-01-16T23:43:20Z

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts

+				loader,
+				provider.driver.createCreateNewRequest(provider.documentId),
+			);
+			provider.updateDocumentId(container.resolvedUrl);


Any specific reason for this 3-step process to create a container rather than just doing provider.createTestContainer(mainContainerConfig)?

Same, consistency purposes. I'll consider making those suggestions to other offline tests as well if there is a strong reason for them.

agarwal-navin · 2025-01-16T23:44:12Z

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts

+			await waitForContainerConnection(container2);
+			await provider.ensureSynchronized();
+
+			assert.strictEqual(map2.get("1"), "1");


Add messages to assert so that when one of these is hit, it's possible to tell which assert was hit. As of now, if this fails in say CI, there will be no way to tell which one of these asserts is hit making debugging harder.

agarwal-navin · 2025-01-16T23:45:16Z

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts

+
+		it("can stash between summary op and ack", async function () {
+			const waitForSummaryPromise = waitForSummary(provider, container, testContainerConfig);
+			const pendingOps = await new Promise<string | undefined>((resolve, reject) =>


Use timeoutPromise or timeoutAwait here in case the summarize op never comes.

agarwal-navin · 2025-01-16T23:46:59Z

packages/test/test-end-to-end-tests/src/test/offline/offlineTestsUtils.ts

+ * @param pendingLocalState - (Optional) custom PendingLocalState to load from. Defaults to using getPendingOps helper if omitted.
+ * @returns A container instance with a connect function to unblock the Driver (simulating coming back from offline)
+ */
+export async function loadOffline(


Maybe call this loadContainerWithDeferredConnection or something along those lines. As of now, this name doesn't align with what it does.

As of now, this name doesn't align with what it does.

Not sure I see your point. What does "load offline" mean to you?

It doesn't really mean anything to me. The fact that it loads a container is not obvious. loadOfflineContainer or loadContainerOffline would be understandable.

agarwal-navin · 2025-01-16T23:48:08Z

packages/test/test-end-to-end-tests/src/test/offline/offlineTestsUtils.ts

+/**
+ * load container, pause, create (local) ops from callback, then optionally send ops before closing container
+ */
+export const getPendingOps = async (


Maybe call this loadContainerAndGetPendingState. It's confusing that getPendingOps would load a container as well.

Or maybe generatePendingState? It needs a live container to do so, but it closes that container before returning, so I wouldn't put loadContainer in the title.

agarwal-navin · 2025-01-16T23:49:32Z

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts

+
+			map1.set("2", "2");
+			await waitForSummary(provider, container, testContainerConfig, summaryVersion);
+			const container2 = await loader.resolve({ url }, pendingOps);


Is it intentional that container2 doesn't load from the summary that happens before this step?

markfields · 2025-01-17T21:18:21Z

packages/test/test-end-to-end-tests/src/test/refreshSerializerLifeCycle.spec.ts

 	type ITestObjectProvider,
 } from "@fluidframework/test-utils/internal";

 import { wrapObjectAndOverride } from "../mocking.js";

+// eslint-disable-next-line import/no-internal-modules


Can't you avoid this by re-exporting under /offline/index.ts? Not sure if there's precedence either way.

Could also consider moving that utils file to another package, probably @fluidframework/test-utils? (tag it as internal)

markfields · 2025-01-17T21:21:41Z

packages/test/test-end-to-end-tests/src/test/refreshSerializerLifeCycle.spec.ts

+				const offlineObject = await loadOffline(
+					testContainerConfig,
+					provider,
+					{ url },
+					pendingOps,
+				);
+				container2 = offlineObject.container;


Nit, if you like it: This avoids having to name offlineObject variable

Suggested change

const offlineObject = await loadOffline(

testContainerConfig,

provider,

{ url },

pendingOps,

);

container2 = offlineObject.container;

const { container } = await loadOffline(

testContainerConfig,

provider,

{ url },

pendingOps,

);

container2 = container;

markfields · 2025-01-17T21:27:43Z

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts

+			url = await container.getAbsoluteUrl("");
+			dataStore1 = (await container.getEntryPoint()) as ITestFluidObject;
+			map1 = await dataStore1.getSharedObject<ISharedMap>(mapId);
+			map1.set("1", "1");


Are you doing this to force a write connection? Nice to put a comment to that effect (some places even do set("force", "write connection"); 🤷‍♂️

markfields · 2025-01-17T21:34:06Z

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts

+
+		it("works with summary while offline", async function () {
+			const summaryVersion = await waitForSummary(provider, container, testContainerConfig);
+			const pendingOps = await getPendingOps(


Am I right that the container used here is totally discarded never to be used again? And not just the container instance but also the backing file that's created?

dannimad added 9 commits December 5, 2024 16:11

wait for summary test

793b6d6

clean summary options

69e0ac1

clean more container config

3096fbb

disable summary for main container again

20b04f2

Navin changes

2d01f9f

Merge branch 'main' into waitforsummary

db241f5

Separate stashed ops tests that wait for a summary.

3804e90

Merge branch 'main' into waitforsummary

87f4309

use offlineTestUtils for other tests

87bf48b

Copilot bot review requested due to automatic review settings January 14, 2025 02:01

github-actions bot added base: main PRs targeted against main branch area: tests Tests to add, test infrastructure improvements, etc labels Jan 14, 2025

Copilot AI reviewed Jan 14, 2025

View reviewed changes

dannimad requested review from markfields, agarwal-navin and anthony-murphy January 14, 2025 18:03

Merge branch 'main' into waitforsummary

69f2735

agarwal-navin reviewed Jan 16, 2025

View reviewed changes

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts Show resolved Hide resolved

agarwal-navin reviewed Jan 16, 2025

View reviewed changes

packages/test/test-end-to-end-tests/src/test/waitForSummary.spec.ts Show resolved Hide resolved

agarwal-navin reviewed Jan 16, 2025

View reviewed changes

markfields reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and reorganize stashed ops that wait for a summary #23542

Fix and reorganize stashed ops that wait for a summary #23542

dannimad commented Jan 14, 2025 •

edited

Loading

Copilot AI left a comment

Copilot AI Jan 14, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

agarwal-navin Jan 16, 2025

dannimad Jan 17, 2025

agarwal-navin Jan 16, 2025

dannimad Jan 17, 2025

agarwal-navin Jan 16, 2025

dannimad Jan 17, 2025

agarwal-navin Jan 16, 2025

agarwal-navin Jan 16, 2025

agarwal-navin Jan 16, 2025

markfields Jan 17, 2025

agarwal-navin Jan 17, 2025

agarwal-navin Jan 16, 2025

markfields Jan 17, 2025

agarwal-navin Jan 16, 2025

markfields Jan 17, 2025

markfields Jan 17, 2025 •

edited

Loading

markfields Jan 17, 2025

markfields Jan 17, 2025

Fix and reorganize stashed ops that wait for a summary #23542

Are you sure you want to change the base?

Fix and reorganize stashed ops that wait for a summary #23542

Conversation

dannimad commented Jan 14, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markfields Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannimad commented Jan 14, 2025 •

edited

Loading

markfields Jan 17, 2025 •

edited

Loading