Skip to content

tree: Add integration-style tests for loading old summary versions#27105

Open
jzaffiro wants to merge 8 commits intomicrosoft:mainfrom
jzaffiro:jzaffiro/summary-load-tests
Open

tree: Add integration-style tests for loading old summary versions#27105
jzaffiro wants to merge 8 commits intomicrosoft:mainfrom
jzaffiro:jzaffiro/summary-load-tests

Conversation

@jzaffiro
Copy link
Copy Markdown
Contributor

@jzaffiro jzaffiro commented Apr 20, 2026

Prevent any future bugs for loading old summaries in newer versions of the code by adding tests for that case.

AB#59669

Copilot AI review requested due to automatic review settings April 20, 2026 22:04
@jzaffiro jzaffiro requested a review from a team as a code owner April 20, 2026 22:04
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an integration-style regression test to ensure current SharedTree code can load summaries written using older minVersionForCollab settings, across compressed/uncompressed tree encoding strategies.

Changes:

  • Introduces summaryLoad.integration.ts to generate per-version summaries and attempt loading them with the current version configuration.
  • Covers multiple FluidClientVersion keys and both TreeCompressionStrategy.Compressed and TreeCompressionStrategy.Uncompressed.

Comment on lines +70 to +73
describe("Summary load regression tests", () => {
before(async () => {
await generateOldSummaries();
});
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

These tests generate the “old” summary files at runtime (in the before hook) using the same code under test. This can mask real regressions because both the writer and reader evolve together, so you’re not actually validating compatibility with summaries produced by prior releases. Consider loading from checked-in fixtures (or snapshots generated once under a --snapshot/regen mode) so the test exercises genuinely historical serialized data.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@jzaffiro jzaffiro Apr 22, 2026

Choose a reason for hiding this comment

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

Addressed by committing snapshot files and adding a regenerate mode separate from the normal test flow

Comment on lines +35 to +38
async function generateOldSummaries(): Promise<void> {
// Clean and recreate the output directory each run
rmSync(outputDir, { recursive: true, force: true });

Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

This test unconditionally deletes and rewrites a directory under src/test (rmSync(outputDir, ...) + writeFileSync(...)). That means a normal test run mutates the working tree (and could delete any checked-in files placed under that directory). Prefer either writing to a per-run temp directory and cleaning it up, or using the existing snapshot infrastructure which only updates files when explicitly regenerating snapshots.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed by committing snapshot files and adding a regenerate mode separate from the normal test flow

MockSharedObjectServices.createFromSummary(JSON.parse(summaryJson)),
stFactory.attributes,
);
assert(tree !== undefined, "Loaded tree should not be undefined");
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

The only assertion is tree !== undefined, but stFactory.load(...) returns a non-optional value, so this doesn’t meaningfully validate that the summary was interpreted correctly. To make this a stronger regression test, assert on the loaded content (e.g., read a view and verify label and the child array values) so semantic decode issues are caught.

Suggested change
assert(tree !== undefined, "Loaded tree should not be undefined");
const view = tree.viewWith(new TreeViewConfiguration({ schema: TestSchema }));
assert.equal(view.root.label, "foo");
assert.deepEqual(
Array.from(view.root.child, (child) => child.count),
[1, 2],
);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated and added a comment to fix assertions if summary content generation is updated.

console.log(`removing snapshot directory: ${outputDir}`);
rmSync(outputDir, { recursive: true, force: true });
}
mkdirSync(outputDir, { recursive: true });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we ever want to remove or update these files. We should only ever add additional variants, since we need to support loading old summaries, even if we don't generate them like that anymore.

So instead of "regenerateOldSummaries" I think we want something like "checkForMissingSummaries(addIfMissing: boolean)"

Rather than deleting all the summaries, it can load all of them (the string contents of the files) into a Set<string>.

Then when we loop through all the summaries we could generate, generate the string we would save to a file.

Then check if that string is in the set: if its not, push it into an array of missing summaries.

Then after the loop, if the array of missing summaries is not empty:
If addIfMissing is false: report an error listing all the ones which are missing.
If addIfMissing is true: write them all to disk with non-colliding names.

To help with the non-colliding names we can include a -1 at the end of each file name, and increment that number if the file already exists.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note that once we do this, it should run as a test, and not the before hook.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Committed summary files and runs a "checkForMissingSummaries" test in addition to the load tests for each summary file in the directory.

Comment thread packages/dds/tree/src/test/shared-tree/summaryLoad.integration.ts Outdated
Comment thread packages/dds/tree/src/test/shared-tree/summaryLoad.integration.ts Outdated
Comment thread packages/dds/tree/src/test/shared-tree/summaryLoad.integration.ts
@github-actions
Copy link
Copy Markdown
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  287071 links
    1898 destination URLs
    2148 URLs ignored
       0 warnings
       0 errors


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants