sharedString,map,cell: Validate event with rollback

[sonalideshpandemsft]

Tests that SharedString correctly raises sequenceDelta events when operations (insert, remove, annotate) are rolled back. The tests ensure that rollback emits the expected deltas and that the resulting text state matches the pre-rollback state.

This change also includes test for map and cell

AB#43938
AB#43938

[Copilot]

Pull Request Overview

This PR adds comprehensive rollback testing for SharedString, SharedMap, and SharedCell DDSes. The tests validate that rollback operations correctly emit expected delta/event notifications and restore proper state when operations (insert, remove, annotate) are rolled back.

• Refactors existing directory rollback tests to use common utilities
• Adds new rollback test utilities (setupRollbackTest, createAdditionalClient) for multi-client scenarios
• Implements extensive test coverage for SharedString rollback scenarios including multi-client coordination

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
packages/dds/test-dds-utils/src/index.ts	Exports new rollback test utilities
packages/dds/test-dds-utils/src/ddsTestUtils.ts	New utility functions for setting up multi-client rollback tests
packages/dds/sequence/src/test/sharedString.rollback.spec.ts	Comprehensive SharedString rollback tests including event validation
packages/dds/map/src/test/mocha/directory.rollback.spec.ts	Refactored to use common utilities and added event validation tests
packages/dds/cell/src/test/cell.rollback.spec.ts	New SharedCell rollback tests with multi-client scenarios

[anthony-murphy]

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.

[anthony-murphy]

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

[ChumpChief]

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.

[ChumpChief]

Mostly just reviewing the infra + cell + directory as I am less familiar with the sequence events.

[ChumpChief]

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.

[ChumpChief]

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.

[ChumpChief]

Seems maybe a little confusing to use the same id for the clientId and the DDS? Probably not problematic but a little bit unintuitive.

[ChumpChief]

Another approach for granularity is to shift the events off as you check them

Suggested change

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

[ChumpChief]

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

[sonalideshpandemsft]

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

[ChumpChief]

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();