feat(sequence): add more tests (microsoft#19398)

Adds some more tests to sequence. Specifically some interval-related
tests from another PR that got killed, and skipped tests related to
grouped batching and reconnect, which currently has a number of crashes.

packages/dds/merge-tree/src/partialLengths.ts

@@ -1185,7 +1185,7 @@ export class PartialSequenceLengths {
 }
 
 /* eslint-disable @typescript-eslint/dot-notation */
-function verifyPartialLengths(
+function verifyPartialLengthsInner(
 	partialSeqLengths: PartialSequenceLengths,
 	partialLengths: PartialSequenceLengthsSet,
 	clientPartials: boolean,
@@ -1266,7 +1266,7 @@ function verifyPartialLengths(
 	return count;
 }
 
-export function verifyExpected(
+export function verifyExpectedPartialLengths(
 	mergeTree: MergeTree,
 	node: IMergeBlock,
 	refSeq: number,
@@ -1305,11 +1305,11 @@ export function verifyExpected(
 	}
 }
 
-export function verify(partialSeqLengths: PartialSequenceLengths) {
+export function verifyPartialLengths(partialSeqLengths: PartialSequenceLengths) {
 	if (partialSeqLengths["clientSeqNumbers"]) {
 		for (const cliSeq of partialSeqLengths["clientSeqNumbers"]) {
 			if (cliSeq) {
-				verifyPartialLengths(partialSeqLengths, cliSeq, true);
+				verifyPartialLengthsInner(partialSeqLengths, cliSeq, true);
 			}
 		}
 
@@ -1319,7 +1319,7 @@ export function verify(partialSeqLengths: PartialSequenceLengths) {
 			0x059 /* "Client view exists but flat view does not!" */,
 		);
 
-		verifyPartialLengths(partialSeqLengths, partialSeqLengths["partialLengths"], false);
+		verifyPartialLengthsInner(partialSeqLengths, partialSeqLengths["partialLengths"], false);
 	} else {
 		// If we don't have a client view, we shouldn't have the flat view either
 		assert(

packages/dds/merge-tree/src/test/index.ts

@@ -22,6 +22,7 @@ export {
 	markRangeRemoved,
 	nodeOrdinalsHaveIntegrity,
 	validatePartialLengths,
+	useStrictPartialLengthChecks,
 } from "./testUtils";
 export {
 	annotateRange,

packages/dds/merge-tree/src/test/obliterate.concurrent.spec.ts

@@ -5,9 +5,9 @@
 
 import { strict as assert } from "assert";
 import { LoggingError } from "@fluidframework/telemetry-utils";
-import { PartialSequenceLengths, verify, verifyExpected } from "../partialLengths";
 import { MergeTree } from "../mergeTree";
 import { ReconnectTestHelper } from "./reconnectHelper";
+import { useStrictPartialLengthChecks } from "./testUtils";
 
 /**
  * Some tests contain ASCII diagrams of the trees to make it easier to reason about
@@ -41,15 +41,13 @@ import { ReconnectTestHelper } from "./reconnectHelper";
 
 for (const incremental of [true, false]) {
 	describe(`obliterate partial lengths incremental = ${incremental}`, () => {
+		useStrictPartialLengthChecks();
+
 		beforeEach(() => {
-			PartialSequenceLengths.options.verifier = verify;
-			PartialSequenceLengths.options.verifyExpected = verifyExpected;
 			MergeTree.options.incrementalUpdate = incremental;
 		});
 
 		afterEach(() => {
-			PartialSequenceLengths.options.verifier = undefined;
-			PartialSequenceLengths.options.verifyExpected = undefined;
 			MergeTree.options.incrementalUpdate = true;
 		});
 

packages/dds/merge-tree/src/test/obliterate.partialLength.spec.ts

@@ -5,19 +5,18 @@
 
 import { strict as assert } from "assert";
 import { MergeTreeDeltaType } from "../ops";
-import { PartialSequenceLengths, verify, verifyExpected } from "../partialLengths";
 import { TestClient } from "./testClient";
-import { insertText, validatePartialLengths } from "./testUtils";
+import { insertText, useStrictPartialLengthChecks, validatePartialLengths } from "./testUtils";
 
 describe("obliterate partial lengths", () => {
 	let client: TestClient;
 	let refSeq: number;
 	const localClientId = 17;
 	const remoteClientId = 18;
 
+	useStrictPartialLengthChecks();
+
 	beforeEach(() => {
-		PartialSequenceLengths.options.verifier = verify;
-		PartialSequenceLengths.options.verifyExpected = verifyExpected;
 		client = new TestClient({
 			mergeTreeEnableObliterate: true,
 		});
@@ -34,10 +33,6 @@ describe("obliterate partial lengths", () => {
 		refSeq = client.getCurrentSeq();
 	});
 
-	afterEach(() => {
-		PartialSequenceLengths.options.verifier = undefined;
-	});
-
 	it("removes text", () => {
 		assert.equal(client.getText(), "hello world");
 		const localObliterateOp = client.obliterateRangeLocal(0, "hello world".length);

packages/dds/merge-tree/src/test/obliterate.reconnect.spec.ts

@@ -4,21 +4,19 @@
  */
 
 import { strict as assert } from "assert";
-import { PartialSequenceLengths, verify, verifyExpected } from "../partialLengths";
 import { MergeTree } from "../mergeTree";
 import { ReconnectTestHelper } from "./reconnectHelper";
+import { useStrictPartialLengthChecks } from "./testUtils";
 
 for (const incremental of [true, false]) {
 	describe(`obliterate partial lengths incremental = ${incremental}`, () => {
+		useStrictPartialLengthChecks();
+
 		beforeEach(() => {
-			PartialSequenceLengths.options.verifier = verify;
-			PartialSequenceLengths.options.verifyExpected = verifyExpected;
 			MergeTree.options.incrementalUpdate = incremental;
 		});
 
 		afterEach(() => {
-			PartialSequenceLengths.options.verifier = undefined;
-			PartialSequenceLengths.options.verifyExpected = undefined;
 			MergeTree.options.incrementalUpdate = true;
 		});
 

packages/dds/merge-tree/src/test/partialLength.spec.ts

@@ -6,19 +6,24 @@
 import { UnassignedSequenceNumber } from "../constants";
 import { MergeTree } from "../mergeTree";
 import { MergeTreeDeltaType } from "../ops";
-import { PartialSequenceLengths, verify, verifyExpected } from "../partialLengths";
 import { TextSegment } from "../textSegment";
-import { insertSegments, insertText, markRangeRemoved, validatePartialLengths } from "./testUtils";
+import {
+	insertSegments,
+	insertText,
+	markRangeRemoved,
+	useStrictPartialLengthChecks,
+	validatePartialLengths,
+} from "./testUtils";
 
 describe("partial lengths", () => {
 	let mergeTree: MergeTree;
 	const localClientId = 17;
 	const remoteClientId = 18;
 	const refSeq = 0;
 
+	useStrictPartialLengthChecks();
+
 	beforeEach(() => {
-		PartialSequenceLengths.options.verifier = verify;
-		PartialSequenceLengths.options.verifyExpected = verifyExpected;
 		mergeTree = new MergeTree();
 		insertSegments({
 			mergeTree,
@@ -33,11 +38,6 @@ describe("partial lengths", () => {
 		mergeTree.startCollaboration(localClientId, /* minSeq: */ 0, /* currentSeq: */ 0);
 	});
 
-	afterEach(() => {
-		PartialSequenceLengths.options.verifier = undefined;
-		PartialSequenceLengths.options.verifyExpected = undefined;
-	});
-
 	it("passes with no additional ops", () => {
 		validatePartialLengths(localClientId, mergeTree, [{ seq: refSeq, len: 12 }]);
 	});

packages/dds/merge-tree/src/test/testUtils.ts

@@ -14,6 +14,11 @@ import { MergeTree } from "../mergeTree";
 import { walkAllChildSegments } from "../mergeTreeNodeWalk";
 import { UnassignedSequenceNumber } from "../constants";
 import { LocalReferenceCollection } from "../localReference";
+import {
+	PartialSequenceLengths,
+	verifyExpectedPartialLengths,
+	verifyPartialLengths,
+} from "../partialLengths";
 import { loadText } from "./text";
 
 export function loadTextFromFile(filename: string, mergeTree: MergeTree, segLimit = 0) {
@@ -267,3 +272,22 @@ export function validateRefCount(collection?: LocalReferenceCollection) {
 	// eslint-disable-next-line @typescript-eslint/dot-notation
 	assert.equal(collection["refCount"], expectedLength);
 }
+
+/**
+ * Enable stricter partial length assertions inside tests
+ *
+ * Note that these assertions can be expensive, and so should not be enabled in
+ * production code or tests that run through thousands of ops (e.g. the SharedString
+ * fuzz tests).
+ */
+export function useStrictPartialLengthChecks() {
+	beforeEach(() => {
+		PartialSequenceLengths.options.verifier = verifyPartialLengths;
+		PartialSequenceLengths.options.verifyExpected = verifyExpectedPartialLengths;
+	});
+
+	afterEach(() => {
+		PartialSequenceLengths.options.verifier = undefined;
+		PartialSequenceLengths.options.verifyExpected = undefined;
+	});
+}

packages/dds/sequence/src/sequence.ts

@@ -594,11 +594,12 @@ export abstract class SharedSegmentSequence<T extends ISegment>
 		const insertIndex: number = Math.max(start, end);
 
 		// Insert first, so local references can slide to the inserted seg if any
-		const insert = this.client.insertSegmentLocal(insertIndex, segment);
-		if (insert) {
-			if (start < end) {
-				this.client.removeRangeLocal(start, end);
-			}
+		const insert = this.guardReentrancy(() =>
+			this.client.insertSegmentLocal(insertIndex, segment),
+		);
+
+		if (insert && start < end) {
+			this.removeRange(start, end);
 		}
 	}
 

packages/dds/sequence/src/sequenceFactory.ts

@@ -63,6 +63,9 @@ export class SharedStringFactory implements IChannelFactory {
 		return sharedString;
 	}
 
+	/**
+	 * {@inheritDoc @fluidframework/datastore-definitions#IChannelFactory.create}
+	 */
 	public create(document: IFluidDataStoreRuntime, id: string): SharedString {
 		const sharedString = new SharedString(document, id, this.attributes);
 		sharedString.initializeLocal();

packages/dds/sequence/src/test/fuzz/sharedString.fuzz.spec.ts

@@ -103,3 +103,31 @@ describe("SharedString fuzz testing with rebased batches", () => {
 		// replay: 0,
 	});
 });
+
+// todo: potentially related to AB#7050
+//
+// `intervalRebasing.spec.ts` contains some reduced tests exhibiting the crashes
+// linked to AB#7050
+describe.skip("SharedString fuzz testing with rebased batches and reconnect", () => {
+	const noReconnectWithRebaseModel = {
+		...baseSharedStringModel,
+		workloadName: "SharedString with rebasing and reconnect",
+	};
+
+	createDDSFuzzSuite(noReconnectWithRebaseModel, {
+		...defaultFuzzOptions,
+		reconnectProbability: 0.3,
+		numberOfClients: 3,
+		clientJoinOptions: {
+			maxNumberOfClients: 3,
+			clientAddProbability: 0.0,
+		},
+		rebaseProbability: 0.2,
+		containerRuntimeOptions: {
+			flushMode: FlushMode.TurnBased,
+			enableGroupedBatching: true,
+		},
+		// Uncomment this line to replay a specific seed from its failure file:
+		// replay: 0,
+	});
+});