Skip to content
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.

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

feat(merge-tree): explicitly test partial lengths #11453

Merged
merged 15 commits into from
Aug 16, 2022

Conversation

connorskees
Copy link
Contributor

Description

More explicitly tests merge-tree partialLength logic. Additionally, moves the verify methods to be free functions.

ado 1434

@github-actions github-actions bot added area: dds Issues related to distributed data structures base: main PRs targeted against main branch labels Aug 8, 2022
@connorskees connorskees force-pushed the feat/partial-length-unit-tests branch from 24d25a3 to dbd03d0 Compare August 9, 2022 17:23
@connorskees connorskees marked this pull request as ready for review August 9, 2022 17:30
@connorskees connorskees requested a review from a team as a code owner August 9, 2022 17:30
Copy link
Contributor

@Abe27342 Abe27342 left a comment

Choose a reason for hiding this comment

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

This looks reasonable as a starting point, but I think there are still interesting cases untested:

  • overlapping remove (two potentially distinct subcases here--remote client 1 and remote client 2 concurrently remove the same segment, then our local client needs to interpret length of the merge block at various seq_s, and the case where it's a remote client being concurrent with the local client)
  • trees that have height more than 2: PartialSequenceLengths.combine has meaningfully different logic when it's merging children leaves vs. when it's merging children that are internal nodes.

I think using code coverage of partialSequenceLengths.ts when only running this test suite is a useful metric to consider here when considering which tests to write.

If you want to work on this iteratively, I'm fine with improving the test suite in a separate PR / working incrementally. Overall state of the PR is fine, I just want to dig a bit deeper :)

@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Aug 9, 2022

@fluid-example/bundle-size-tests: -231 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 392.36 KB 392.28 KB -77 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 191.92 KB 191.92 KB No change
loader.js 151.12 KB 151.12 KB No change
map.js 42.63 KB 42.63 KB No change
matrix.js 131.7 KB 131.63 KB -77 Bytes
odspDriver.js 150.23 KB 150.23 KB No change
odspPrefetchSnapshot.js 38.39 KB 38.39 KB No change
sharedString.js 152.34 KB 152.26 KB -77 Bytes
Total Size 1.25 MB 1.25 MB -231 Bytes

Baseline commit: 0026561

Generated by 🚫 dangerJS against 9081e16

@connorskees connorskees requested a review from Abe27342 August 10, 2022 18:07
@connorskees connorskees requested a review from Abe27342 August 15, 2022 14:03
Copy link
Contributor

@Abe27342 Abe27342 left a comment

Choose a reason for hiding this comment

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

Great, glad to have this extra coverage--I have the one minor nit on that edge case, but honestly just putting a comment there mentioning it doesn't work for that case would be ok.

@connorskees connorskees merged commit a972870 into microsoft:main Aug 16, 2022
@github-actions
Copy link
Contributor

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds Issues related to distributed data structures base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants