From a972870b712dd815508ed45f4fa3321eb6d64eb0 Mon Sep 17 00:00:00 2001 From: Connor Skees <39542938+connorskees@users.noreply.github.com> Date: Tue, 16 Aug 2022 13:54:00 -0400 Subject: [PATCH] feat(merge-tree): explicitly test partial lengths (#11453) --- packages/dds/merge-tree/src/partialLengths.ts | 140 ++++---- .../merge-tree/src/test/partialLength.spec.ts | 339 ++++++++++++++++++ 2 files changed, 412 insertions(+), 67 deletions(-) create mode 100644 packages/dds/merge-tree/src/test/partialLength.spec.ts diff --git a/packages/dds/merge-tree/src/partialLengths.ts b/packages/dds/merge-tree/src/partialLengths.ts index 19e5fcce14a0..987da091d3c1 100644 --- a/packages/dds/merge-tree/src/partialLengths.ts +++ b/packages/dds/merge-tree/src/partialLengths.ts @@ -263,7 +263,7 @@ export class PartialSequenceLengths { } if (PartialSequenceLengths.options.verify) { - combinedPartialLengths.verify(); + verify(combinedPartialLengths); } return combinedPartialLengths; @@ -328,7 +328,7 @@ export class PartialSequenceLengths { } if (PartialSequenceLengths.options.verify) { - combinedPartialLengths.verify(); + verify(combinedPartialLengths); } return combinedPartialLengths; } @@ -611,7 +611,7 @@ export class PartialSequenceLengths { this.zamboni(collabWindow); } if (PartialSequenceLengths.options.verify) { - this.verify(); + verify(this); } } @@ -817,83 +817,89 @@ export class PartialSequenceLengths { return -1; } } +} - // Debug only - private verifyPartialLengths(partialLengths: PartialSequenceLength[], clientPartials: boolean) { - if (partialLengths.length === 0) { return 0; } - - let lastSeqNum = 0; - let accumSegLen = 0; - let count = 0; - - for (const partialLength of partialLengths) { - // Count total number of partial length - count++; - - // Sequence number should be larger or equal to minseq - assert(this.minSeq <= partialLength.seq, 0x054 /* "Sequence number less than minSeq!" */); - - // Sequence number should be sorted - assert(lastSeqNum < partialLength.seq, 0x055 /* "Sequence number is not sorted!" */); - lastSeqNum = partialLength.seq; - - // Len is a accumulation of all the seglen adjustments - accumSegLen += partialLength.seglen; - if (accumSegLen !== partialLength.len) { - assert(false, 0x056 /* "Unexpected total for accumulation of all seglen adjustments!" */); - } +/* eslint-disable @typescript-eslint/dot-notation */ +function verifyPartialLengths( + partialSeqLengths: PartialSequenceLengths, + partialLengths: PartialSequenceLength[], + clientPartials: boolean, +) { + if (partialLengths.length === 0) { return 0; } + + let lastSeqNum = 0; + let accumSegLen = 0; + let count = 0; + + for (const partialLength of partialLengths) { + // Count total number of partial length + count++; + + // Sequence number should be larger or equal to minseq + assert(partialSeqLengths.minSeq <= partialLength.seq, 0x054 /* "Sequence number less than minSeq!" */); + + // Sequence number should be sorted + assert(lastSeqNum < partialLength.seq, 0x055 /* "Sequence number is not sorted!" */); + lastSeqNum = partialLength.seq; + + // Len is a accumulation of all the seglen adjustments + accumSegLen += partialLength.seglen; + if (accumSegLen !== partialLength.len) { + assert(false, 0x056 /* "Unexpected total for accumulation of all seglen adjustments!" */); + } - if (clientPartials) { - // Client partials used to track local edits so we can account for them some refSeq. - // But the information we keep track of are since minSeq, so we keep track of more history - // then needed, and some of them doesn't make sense to be used for length calculations - // e.g. if you have this sequence, where the minSeq is #5 because of other clients - // seq 10: client 1: insert seg #1 - // seq 11: client 2: delete seg #2 refseq: 10 - // minLength is 0, we would have keep a record of seglen: -1 for clientPartialLengths for client 2 - // So if you ask for partial length for client 2 @ seq 5, we will have return -1. - // However, that combination is invalid, since we should never see any ops with refseq < 10 for - // client 2 after seq 11. - } else { - // Len adjustment should not make length negative - if (this.minLength + partialLength.len < 0) { - assert(false, 0x057 /* "Negative length after length adjustment!" */); - } + if (clientPartials) { + // Client partials used to track local edits so we can account for them some refSeq. + // But the information we keep track of are since minSeq, so we keep track of more history + // then needed, and some of them doesn't make sense to be used for length calculations + // e.g. if you have this sequence, where the minSeq is #5 because of other clients + // seq 10: client 1: insert seg #1 + // seq 11: client 2: delete seg #2 refseq: 10 + // minLength is 0, we would have keep a record of seglen: -1 for clientPartialLengths for client 2 + // So if you ask for partial length for client 2 @ seq 5, we will have return -1. + // However, that combination is invalid, since we should never see any ops with refseq < 10 for + // client 2 after seq 11. + } else { + // Len adjustment should not make length negative + if (partialSeqLengths["minLength"] + partialLength.len < 0) { + assert(false, 0x057 /* "Negative length after length adjustment!" */); } + } - if (partialLength.overlapRemoveClients) { - // Only the flat partialLengths can have overlapRemoveClients, the per client view shouldn't - assert(!clientPartials, 0x058 /* "Both overlapRemoveClients and clientPartials are set!" */); + if (partialLength.overlapRemoveClients) { + // Only the flat partialLengths can have overlapRemoveClients, the per client view shouldn't + assert(!clientPartials, 0x058 /* "Both overlapRemoveClients and clientPartials are set!" */); - // Each overlap client count as one - count += partialLength.overlapRemoveClients.size(); - } + // Each overlap client count as one, but the first remove to sequence was already counted. + // (this aligns with the logic to omit the removing client in `addClientSeqNumberFromPartial`) + count += partialLength.overlapRemoveClients.size() - 1; } - return count; } + return count; +} - private verify() { - if (this.clientSeqNumbers) { - let cliCount = 0; - for (const cliSeq of this.clientSeqNumbers) { - if (cliSeq) { - cliCount += this.verifyPartialLengths(cliSeq, true); - } +function verify(partialSeqLengths: PartialSequenceLengths) { + if (partialSeqLengths["clientSeqNumbers"]) { + let cliCount = 0; + for (const cliSeq of partialSeqLengths["clientSeqNumbers"]) { + if (cliSeq) { + cliCount += verifyPartialLengths(partialSeqLengths, cliSeq, true); } + } - // If we have client view, we should have the flat view - assert(!!this.partialLengths, 0x059 /* "Client view exists but flat view does not!" */); - const flatCount = this.verifyPartialLengths(this.partialLengths, false); + // If we have client view, we should have the flat view + assert(!!partialSeqLengths["partialLengths"], 0x059 /* "Client view exists but flat view does not!" */); + const flatCount = verifyPartialLengths(partialSeqLengths, partialSeqLengths["partialLengths"], false); - // The number of partial lengths on the client view and flat view should be the same - assert(flatCount === cliCount, - 0x05a /* "Mismatch between number of partial lengths on client and flat views!" */); - } else { - // If we don't have a client view, we shouldn't have the flat view either - assert(!this.partialLengths, 0x05b /* "Flat view exists but client view does not!" */); - } + // The number of partial lengths on the client view and flat view should be the same + assert(flatCount === cliCount, + 0x05a /* "Mismatch between number of partial lengths on client and flat views!" */); + } else { + // If we don't have a client view, we shouldn't have the flat view either + assert(!partialSeqLengths["partialLengths"], 0x05b /* "Flat view exists but client view does not!" */); } } +/* eslint-enable @typescript-eslint/dot-notation */ /** * Clones an `overlapRemoveClients` red-black tree. diff --git a/packages/dds/merge-tree/src/test/partialLength.spec.ts b/packages/dds/merge-tree/src/test/partialLength.spec.ts new file mode 100644 index 000000000000..e221ced0b15f --- /dev/null +++ b/packages/dds/merge-tree/src/test/partialLength.spec.ts @@ -0,0 +1,339 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import { strict as assert } from "assert"; +import { UnassignedSequenceNumber } from "../constants"; +import { MergeTree } from "../mergeTree"; +import { MergeTreeDeltaType } from "../ops"; +import { PartialSequenceLengths } from "../partialLengths"; +import { TextSegment } from "../textSegment"; +import { insertText } from "./testUtils"; + +describe("partial lengths", () => { + let mergeTree: MergeTree; + const localClientId = 17; + const remoteClientId = 18; + + function getPartialLengths( + clientId: number, + seq: number, + mergeBlock = mergeTree.root, + ) { + const partialLen = mergeBlock.partialLengths?.getPartialLength( + seq, + clientId, + ); + + let actualLen = 0; + + mergeTree.walkAllSegments(mergeBlock, (segment) => { + // this condition does not account for un-acked changes + if ( + segment.isLeaf() + && !(segment.removedSeq !== undefined && segment.removedSeq >= seq) + && segment.localRemovedSeq === undefined + && (segment.seq === undefined || segment.seq <= seq) + ) { + actualLen += segment.cachedLength; + } + return true; + }); + + return { + partialLen, + actualLen, + }; + } + + function validatePartialLengths( + clientId: number, + expectedValues?: [{ seq: number; len: number; }], + mergeBlock = mergeTree.root, + ): void { + for (let i = mergeTree.collabWindow.minSeq + 1; i <= mergeTree.collabWindow.currentSeq; i++) { + const { partialLen, actualLen } = getPartialLengths(clientId, i, mergeBlock); + + assert.equal(partialLen, actualLen); + } + + if (!expectedValues) { + return; + } + + for (const { seq, len } of expectedValues) { + const { partialLen, actualLen } = getPartialLengths(clientId, seq, mergeBlock); + + assert.equal(partialLen, len); + assert.equal(actualLen, len); + } + } + + beforeEach(() => { + PartialSequenceLengths.options.verify = true; + mergeTree = new MergeTree(); + mergeTree.insertSegments( + 0, + [TextSegment.make("hello world!")], + 0, + localClientId, + 0, + undefined); + + mergeTree.startCollaboration( + localClientId, + /* minSeq: */ 0, + /* currentSeq: */ 0); + }); + + afterEach(() => { + PartialSequenceLengths.options.verify = false; + }); + + it("passes with no additional ops", () => { + validatePartialLengths(localClientId, [{ seq: 0, len: 12 }]); + }); + + describe("a single inserted element", () => { + it("includes length of local insert for local view", () => { + insertText( + mergeTree, + 0, + 0, + localClientId, + 1, + "more ", + undefined, + { op: { type: MergeTreeDeltaType.INSERT } }, + ); + + validatePartialLengths(localClientId, [{ seq: 1, len: 17 }]); + }); + it("includes length of local insert for remote view", () => { + insertText( + mergeTree, + 0, + 0, + localClientId, + 1, + "more ", + undefined, + { op: { type: MergeTreeDeltaType.INSERT } }, + ); + + validatePartialLengths(remoteClientId, [{ seq: 1, len: 17 }]); + }); + it("includes length of remote insert for local view", () => { + insertText( + mergeTree, + 0, + 0, + remoteClientId, + 1, + "more ", + undefined, + { op: { type: MergeTreeDeltaType.INSERT } }, + ); + + validatePartialLengths(localClientId, [{ seq: 1, len: 17 }]); + }); + it("includes length of remote insert for remote view", () => { + insertText( + mergeTree, + 0, + 0, + remoteClientId, + 1, + "more ", + undefined, + { op: { type: MergeTreeDeltaType.INSERT } }, + ); + + validatePartialLengths(remoteClientId, [{ seq: 1, len: 17 }]); + }); + }); + + describe("a single removed segment", () => { + it("includes result of local delete for local view", () => { + mergeTree.markRangeRemoved( + 0, + 12, + 0, + localClientId, + 1, + false, + undefined as any); + + validatePartialLengths(localClientId, [{ seq: 0, len: 0 }]); + }); + it("includes result of local delete for remote view", () => { + mergeTree.markRangeRemoved( + 0, + 12, + 0, + localClientId, + 1, + false, + undefined as any); + + validatePartialLengths(remoteClientId, [{ seq: 1, len: 0 }]); + }); + it("includes result of remote delete for local view", () => { + mergeTree.markRangeRemoved( + 0, + 12, + 0, + remoteClientId, + 1, + false, + undefined as any); + + validatePartialLengths(localClientId, [{ seq: 1, len: 0 }]); + }); + it("includes result of remote delete for remote view", () => { + mergeTree.markRangeRemoved( + 0, + 12, + 0, + remoteClientId, + 1, + false, + undefined as any); + + validatePartialLengths(remoteClientId, [{ seq: 0, len: 0 }]); + }); + }); + + describe("aggregation", () => { + it("includes lengths from multiple permutations in single tree", () => { + mergeTree.insertSegments( + 0, + [TextSegment.make("1")], + 0, + localClientId, + 1, + undefined, + ); + mergeTree.insertSegments( + 0, + [TextSegment.make("2")], + 1, + remoteClientId, + 2, + undefined, + ); + mergeTree.insertSegments( + 0, + [TextSegment.make("3")], + 2, + localClientId, + 3, + undefined, + ); + mergeTree.insertSegments( + 0, + [TextSegment.make("4")], + 3, + remoteClientId, + 4, + undefined, + ); + + validatePartialLengths(localClientId, [{ seq: 4, len: 16 }]); + validatePartialLengths(remoteClientId, [{ seq: 4, len: 16 }]); + }); + + it("is correct for different heights", () => { + for (let i = 0; i < 100; i++) { + insertText( + mergeTree, + 0, + i, + localClientId, + i + 1, + "a", + undefined, + { op: { type: MergeTreeDeltaType.INSERT } }, + ); + + validatePartialLengths(localClientId, [{ seq: i + 1, len: i + 13 }]); + validatePartialLengths(remoteClientId, [{ seq: i + 1, len: i + 13 }]); + } + + validatePartialLengths(localClientId, [{ seq: 100, len: 112 }]); + validatePartialLengths(remoteClientId, [{ seq: 100, len: 112 }]); + }); + }); + + describe("concurrent, overlapping deletes", () => { + it("concurrent remote changes are visible to local", () => { + mergeTree.markRangeRemoved( + 0, + 10, + 0, + remoteClientId, + 1, + false, + undefined as any, + ); + mergeTree.markRangeRemoved( + 0, + 10, + 0, + remoteClientId + 1, + 2, + false, + undefined as any, + ); + + validatePartialLengths(localClientId, [{ seq: 1, len: 2 }]); + }); + it("concurrent local and remote changes are visible", () => { + mergeTree.markRangeRemoved( + 0, + 10, + 0, + localClientId, + 1, + false, + undefined as any, + ); + mergeTree.markRangeRemoved( + 0, + 10, + 0, + remoteClientId, + 2, + false, + undefined as any, + ); + + validatePartialLengths(localClientId, [{ seq: 0, len: 2 }]); + validatePartialLengths(remoteClientId, [{ seq: 0, len: 2 }]); + }); + it("concurrent remote and unsequenced local changes are visible", () => { + mergeTree.markRangeRemoved( + 0, + 10, + 0, + localClientId, + UnassignedSequenceNumber, + false, + undefined as any, + ); + mergeTree.markRangeRemoved( + 0, + 10, + 0, + remoteClientId, + 2, + false, + undefined as any, + ); + + validatePartialLengths(localClientId, [{ seq: 0, len: 2 }]); + validatePartialLengths(remoteClientId, [{ seq: 0, len: 2 }]); + }); + }); +});