Skip to content

sharedString,map,cell: Validate event with rollback #25300

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 13 commits into
base: main
Choose a base branch
from
+1,496 −71
Open

sharedString,map,cell: Validate event with rollback #25300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
40c572d
wip
sonalideshpandemsft Aug 18, 2025
9bda8bb
Merge remote-tracking branch 'origin' into rollback
sonalideshpandemsft Aug 19, 2025
4078565
Add basic tests
sonalideshpandemsft Aug 19, 2025
8617906
sequenceDelta
sonalideshpandemsft Aug 19, 2025
b54d016
Add tests for multiple clients
sonalideshpandemsft Aug 20, 2025
9451072
nits
sonalideshpandemsft Aug 21, 2025
4b50116
Rollback tests for cell
sonalideshpandemsft Aug 22, 2025
e0fd9f0
move helper functions
sonalideshpandemsft Aug 25, 2025
80f07d8
use helper
sonalideshpandemsft Aug 25, 2025
d8fb934
Merge remote-tracking branch 'origin' into check-events-with-rollback
sonalideshpandemsft Aug 25, 2025
6998f8c
feedback
sonalideshpandemsft Aug 26, 2025
3d58f53
intervalcollection: wip
sonalideshpandemsft Aug 26, 2025
e4ec91a
feedback
sonalideshpandemsft Sep 1, 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
173 changes: 173 additions & 0 deletions packages/dds/cell/src/test/cell.rollback.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

import { strict as assert } from "node:assert";

import { setupRollbackTest, createAdditionalClient } from "@fluid-private/test-dds-utils";

import { CellFactory } from "../cellFactory.js";
import type { ISharedCell } from "../interfaces.js";

const cellFactory = new CellFactory();

describe("Cell with rollback", () => {
it("should emit valueChanged on set and rollback should re-emit previous value", async () => {
const { dds: cell, containerRuntime } = setupRollbackTest<ISharedCell>(
"cell-1",
(rt, id): ISharedCell => cellFactory.create(rt, id),
);

const events: (string | undefined)[] = [];

cell.on("valueChanged", (value) => events.push("valueChanged"));
Copy link
Contributor

@ChumpChief ChumpChief Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

These tests should also test event callback params, as these are particularly interesting in rollback cases (they need to reflect the inverse operation as well, which sometimes requires special bookkeeping in the DDS). Consider instead of just having an array of strings for the events, push some structure that also includes the args that the callback received (example with map). EDIT: I just realized cell's event params are different and maybe less interesting since they don't provide the previousValue, but still good to verify as a practice.


cell.on("delete", () => events.push("delete"));

cell.set(10);
assert.equal(cell.get(), 10);

containerRuntime.rollback?.();

assert.equal(cell.get(), undefined);

assert.deepEqual(events, ["valueChanged", "delete"]);
});

it("should emit delete on delete, and rollback should re-emit last valueChanged", async () => {
const {
dds: cell,
containerRuntimeFactory,
containerRuntime,
} = setupRollbackTest<ISharedCell>(
"cell-1",
(rt, id): ISharedCell => cellFactory.create(rt, id),
);

const events: (string | undefined)[] = [];

cell.on("valueChanged", (value) => events.push("valueChanged"));
cell.on("delete", () => events.push("delete"));

cell.set(42);
assert.equal(events.shift(), "valueChanged");
containerRuntime.flush();
containerRuntimeFactory.processAllMessages();

cell.delete();
assert(cell.empty());

// rollback delete
containerRuntime.rollback?.();
assert.equal(cell.get(), 42);

// delete triggers delete event, rollback restores valueChanged
assert.deepEqual(events, ["valueChanged", "delete", "valueChanged"]);
Copy link
Contributor

Choose a reason for hiding this comment

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

i think these tests would be easier to follow if they had more granular event validation, rather than doing it all at the end. to make that easier we could add a helper method like

function captureEvents(events: string[], emitter: {on:(e: string, l: ()=>void)=>void,off:(e: string, l: ()=>void)=>void}, act:()=>void){
	const fired: string[]=[];
	const handlers = new Map<string, ()=>void>();
	for(const event of events){
		const handler = ()=>fired.push(event)
		emitter.on(event,handler);
		handlers.set(event, handler);
	}
	try{
		act();
	}finally{
		for(const [event,handler] of handlers){
			emitter.off(event, handler)
		}
	}
	return fired;
}

this would make it easy to get the events per action, and validate per action, rather than doing it all at the end.

this would be quite a bit of refactoring. i'd probably try writing the helper method manually. migrating one test in the file, and then seeing if copilot could refactor the rest. it might save some time. be sure to commit incrementally, as copilot can also go off the rails, and you don't want to lose work.

Copy link
Contributor

Choose a reason for hiding this comment

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

this might be hard to do for the more advance tests, i think could help with the simper test

Copy link
Contributor

Choose a reason for hiding this comment

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

I light disagree with this approach, in particular with the act() callback here. Not a blocker to me either way but I'd prefer the PR's current approach.

Agree granularity is nice but is achieved more easily and blatantly obvious to a future reader by just clearing out the events array periodically.

Or if we do ultimately want a more reusable approach, the best option is probably to use an existing testing library's function mocks or spies rather than writing our own, something like toHaveBeenNthCalledWith would probably be a natural fit for this scenario. I'm less familiar with chai/sinon but I'm sure their options are fine too.

});
});

describe("SharedCell rollback events with multiple clients", () => {
it("should emit valueChanged on set and rollback should re-emit previous value across clients", async () => {
// Setup two clients
const {
dds: cell1,
containerRuntimeFactory,
containerRuntime: runtime1,
} = setupRollbackTest<ISharedCell>(
"client-1",
(rt, id): ISharedCell => cellFactory.create(rt, id),
);
const { dds: cell2 } = createAdditionalClient(
containerRuntimeFactory,
"client-2",
(rt, id): ISharedCell => cellFactory.create(rt, `cell-${id}`),
);

const events1: string[] = [];
const events2: string[] = [];

// Listen to valueChanged events on both clients
cell1.on("valueChanged", () => events1.push("valueChanged"));
cell2.on("valueChanged", () => events2.push("valueChanged"));

// Client 1 sets a value
cell1.set(10);
assert.equal(cell1.get(), 10);

// Propagate ops to client 2
runtime1.flush();
containerRuntimeFactory.processAllMessages();

assert.equal(cell2.get(), 10);

cell1.set(100);
assert.equal(cell1.get(), 100);
assert.equal(cell2.get(), 10);

// Rollback on client 1
runtime1.rollback?.();

assert.equal(cell1.get(), 10);
assert.equal(cell2.get(), 10);

// Both clients should have seen the events
assert.deepEqual(events1, ["valueChanged", "valueChanged", "valueChanged"]);
assert.deepEqual(events2, ["valueChanged"]);
});

it("should emit delete on delete, and rollback should re-emit last valueChanged across clients", async () => {
// Setup two clients
const {
dds: cell1,
containerRuntimeFactory,
containerRuntime: runtime1,
} = setupRollbackTest<ISharedCell>(
"client-1",
(rt, id): ISharedCell => cellFactory.create(rt, id),
);
const { dds: cell2 } = createAdditionalClient(
containerRuntimeFactory,
"client-2",
(rt, id): ISharedCell => cellFactory.create(rt, `cell-${id}`),
);

const events1: string[] = [];
const events2: string[] = [];

// Attach listeners
cell1.on("valueChanged", () => events1.push("valueChanged"));
cell1.on("delete", () => events1.push("delete"));

cell2.on("valueChanged", () => events2.push("valueChanged"));
cell2.on("delete", () => events2.push("delete"));
Comment on lines +120 to +143
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of this repeated setup would probably be a good candidate to move to a beforeEach block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was removed in the previous iteration as per: #25300 (comment).

Overall it seems the test is easier to follow with the initialization kept inside the test itself, even though some setup is repeated


// Set initial value and propagate
cell1.set(42);
runtime1.flush();
containerRuntimeFactory.processAllMessages();

assert.equal(cell2.get(), 42);

// Delete the value
cell1.delete();

assert(cell1.empty());
assert.equal(cell2.get(), 42);

// Rollback delete
runtime1.rollback?.();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would probably be good to do a final flush/process (that we expect to be a no-op) to verify nothing slips out that ends up in cell2/events2.

Suggested change
runtime1.rollback?.();
runtime1.rollback?.();
// After rollback, this flush/process should not affect cell2.
runtime1.flush();
containerRuntimeFactory.processAllMessages();


// After rollback, this flush/process should not affect cell2.
runtime1.flush();
containerRuntimeFactory.processAllMessages();

// After rollback, value is restored
assert.equal(cell1.get(), 42);
assert.equal(cell2.get(), 42);

// Event order
assert.deepEqual(events1, ["valueChanged", "delete", "valueChanged"]);
assert.deepEqual(events2, ["valueChanged"]);
});
});
146 changes: 75 additions & 71 deletions packages/dds/map/src/test/mocha/directory.rollback.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,68 +5,20 @@

import { strict as assert } from "node:assert";

import { AttachState } from "@fluidframework/container-definitions";
import {
MockContainerRuntimeFactory,
MockFluidDataStoreRuntime,
MockStorage,
type MockContainerRuntime,
} from "@fluidframework/test-runtime-utils/internal";
import { setupRollbackTest, createAdditionalClient } from "@fluid-private/test-dds-utils";

import { DirectoryFactory } from "../../directoryFactory.js";
import type { ISharedDirectory, IValueChanged } from "../../interfaces.js";

interface RollbackTestSetup {
sharedDirectory: ISharedDirectory;
dataStoreRuntime: MockFluidDataStoreRuntime;
containerRuntimeFactory: MockContainerRuntimeFactory;
containerRuntime: MockContainerRuntime;
}

const directoryFactory = new DirectoryFactory();

function setupRollbackTest(): RollbackTestSetup {
const containerRuntimeFactory = new MockContainerRuntimeFactory({ flushMode: 1 }); // TurnBased
const dataStoreRuntime = new MockFluidDataStoreRuntime({ clientId: "1" });
const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataStoreRuntime);
const sharedDirectory = directoryFactory.create(dataStoreRuntime, "shared-directory-1");
dataStoreRuntime.setAttachState(AttachState.Attached);
sharedDirectory.connect({
deltaConnection: dataStoreRuntime.createDeltaConnection(),
objectStorage: new MockStorage(),
});
return {
sharedDirectory,
dataStoreRuntime,
containerRuntimeFactory,
containerRuntime,
};
}

// Helper to create another client attached to the same containerRuntimeFactory
function createAdditionalClient(
containerRuntimeFactory: MockContainerRuntimeFactory,
id: string = "client-2",
): {
sharedDirectory: ISharedDirectory;
dataStoreRuntime: MockFluidDataStoreRuntime;
containerRuntime: MockContainerRuntime;
} {
const dataStoreRuntime = new MockFluidDataStoreRuntime({ clientId: id });
const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataStoreRuntime);
const sharedDirectory = directoryFactory.create(dataStoreRuntime, `shared-directory-${id}`);
dataStoreRuntime.setAttachState(AttachState.Attached);
sharedDirectory.connect({
deltaConnection: dataStoreRuntime.createDeltaConnection(),
objectStorage: new MockStorage(),
});
return { sharedDirectory, dataStoreRuntime, containerRuntime };
}

describe("SharedDirectory rollback", () => {
describe("Storage operations (root subdirectory)", () => {
it("should rollback set operation", () => {
const { sharedDirectory, containerRuntime } = setupRollbackTest();
const { dds: sharedDirectory, containerRuntime } = setupRollbackTest<ISharedDirectory>(
"client-1",
(rt, id): ISharedDirectory => directoryFactory.create(rt, id),
);
const valueChanges: IValueChanged[] = [];
sharedDirectory.on("valueChanged", (event: IValueChanged) => {
valueChanges.push(event);
Expand Down Expand Up @@ -94,8 +46,14 @@ describe("SharedDirectory rollback", () => {
});

it("should rollback delete operation", () => {
const { sharedDirectory, containerRuntimeFactory, containerRuntime } =
setupRollbackTest();
const {
dds: sharedDirectory,
containerRuntime,
containerRuntimeFactory,
} = setupRollbackTest<ISharedDirectory>(
"client-1",
(rt, id): ISharedDirectory => directoryFactory.create(rt, id),
);
sharedDirectory.set("key1", "value1");
containerRuntime.flush();
containerRuntimeFactory.processAllMessages();
Expand Down Expand Up @@ -126,8 +84,14 @@ describe("SharedDirectory rollback", () => {
});

it("should rollback clear operation", () => {
const { sharedDirectory, containerRuntimeFactory, containerRuntime } =
setupRollbackTest();
const {
dds: sharedDirectory,
containerRuntime,
containerRuntimeFactory,
} = setupRollbackTest<ISharedDirectory>(
"client-1",
(rt, id): ISharedDirectory => directoryFactory.create(rt, id),
);
sharedDirectory.set("key1", "value1");
sharedDirectory.set("key2", "value2");
containerRuntime.flush();
Expand Down Expand Up @@ -182,8 +146,14 @@ describe("SharedDirectory rollback", () => {
});

it("should rollback multiple operations in sequence", () => {
const { sharedDirectory, containerRuntimeFactory, containerRuntime } =
setupRollbackTest();
const {
dds: sharedDirectory,
containerRuntime,
containerRuntimeFactory,
} = setupRollbackTest<ISharedDirectory>(
"client-1",
(rt, id): ISharedDirectory => directoryFactory.create(rt, id),
);
sharedDirectory.set("key1", "value1");
sharedDirectory.set("key2", "value2");
containerRuntime.flush();
Expand Down Expand Up @@ -259,11 +229,22 @@ describe("SharedDirectory rollback", () => {
});

it("should rollback local changes in presence of remote changes from another client", () => {
const { sharedDirectory, containerRuntimeFactory, containerRuntime } =
setupRollbackTest();
const {
dds: sharedDirectory,
containerRuntime,
containerRuntimeFactory,
} = setupRollbackTest<ISharedDirectory>(
"client-1",
(rt, id): ISharedDirectory => directoryFactory.create(rt, id),
);
// Create a second client
const { sharedDirectory: sharedDirectory2, containerRuntime: containerRuntime2 } =
createAdditionalClient(containerRuntimeFactory);

const { dds: sharedDirectory2, containerRuntime: containerRuntime2 } =
createAdditionalClient(
containerRuntimeFactory,
"client-2",
(rt, id): ISharedDirectory => directoryFactory.create(rt, `directory-${id}`),
);

sharedDirectory.set("key1", "value1");
sharedDirectory.set("key2", "value2");
Expand Down Expand Up @@ -307,8 +288,14 @@ describe("SharedDirectory rollback", () => {

describe("Storage operations (nested subdirectories)", () => {
it("should rollback all basic operations (set, delete, clear) in subdirectories and nested subdirectories", () => {
const { sharedDirectory, containerRuntimeFactory, containerRuntime } =
setupRollbackTest();
const {
dds: sharedDirectory,
containerRuntime,
containerRuntimeFactory,
} = setupRollbackTest<ISharedDirectory>(
"client-1",
(rt, id): ISharedDirectory => directoryFactory.create(rt, id),
);

const subDir = sharedDirectory.createSubDirectory("subdir");
const level1 = sharedDirectory.createSubDirectory("level1");
Expand Down Expand Up @@ -383,10 +370,21 @@ describe("SharedDirectory rollback", () => {
});

it("should rollback subdirectory operations with concurrent remote changes", () => {
const { sharedDirectory, containerRuntimeFactory, containerRuntime } =
setupRollbackTest();
const { sharedDirectory: sharedDirectory2, containerRuntime: containerRuntime2 } =
createAdditionalClient(containerRuntimeFactory);
const {
dds: sharedDirectory,
containerRuntime,
containerRuntimeFactory,
} = setupRollbackTest<ISharedDirectory>(
"client-1",
(rt, id): ISharedDirectory => directoryFactory.create(rt, id),
);

const { dds: sharedDirectory2, containerRuntime: containerRuntime2 } =
createAdditionalClient(
containerRuntimeFactory,
"client-2",
(rt, id): ISharedDirectory => directoryFactory.create(rt, `directory-${id}`),
);

const subDir1 = sharedDirectory.createSubDirectory("shared");
const nestedDir1 = subDir1.createSubDirectory("nested");
Expand Down Expand Up @@ -443,8 +441,14 @@ describe("SharedDirectory rollback", () => {
});

it("should rollback complex mixed operations across multiple subdirectory levels", () => {
const { sharedDirectory, containerRuntimeFactory, containerRuntime } =
setupRollbackTest();
const {
dds: sharedDirectory,
containerRuntime,
containerRuntimeFactory,
} = setupRollbackTest<ISharedDirectory>(
"client-1",
(rt, id): ISharedDirectory => directoryFactory.create(rt, id),
);

const dirA = sharedDirectory.createSubDirectory("dirA");
const dirB = sharedDirectory.createSubDirectory("dirB");
Expand Down
Loading
Loading