Ensure sufficient test coverage around recent changes to compression/grouping config in tests #23693
Conversation
github-actions
bot
added
area: tests
MarioJGMsoft
requested review from
a team,
pragya91,
markfields,
jason-ha,
jatgarg,
kian-thompson,
WillieHabi and
rajatch-ff
and removed request for
a team
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/test/test-end-to-end-tests/src/test/messageSize.spec.ts:1
- The function should ensure that sizeInBytes is even before calling crypto.randomBytes to prevent potential runtime errors.
generateRandomStringOfSize = (sizeInBytes: number): string =>
| @@ -1264,7 +1264,34 @@ describe("Runtime", () => { | |||
| "Ops with unrecognized type should fail to process", | |||
| ); | |||
| }); | |||
|
|
|||
| it.only("Throws when op compression is on and op grouping is off", async () => { | |||
There was a problem hiding this comment.
Build (lint) fails with this saying only is not allowed, right?
There was a problem hiding this comment.
Yes, I accidentally missed that, thanks for pointing it out!
| localMap.set(`key${i}`, content); | ||
| } | ||
| itExpects( | ||
| `Batch with 4000 ops - grouped batches`, |
There was a problem hiding this comment.
What do you think - would this be worth testing with both grouping and compression disabled? If it feels worth it to you, maybe pull it out and do the [true, false].forEach thing. If not, then just leave it.
There was a problem hiding this comment.
I feel like it isn't, because in the original flow, it used to compress those 4000 ops with the empty placeholders, but since we no longer support that flow I find it unnecessary to create a test where both are disabled.
| @@ -1268,7 +1268,34 @@ describe("Runtime", () => { | |||
| "Ops with unrecognized type should fail to process", | |||
| ); | |||
| }); | |||
|
|
|||
| it("Throws when op compression is on and op grouping is off", async () => { | |||
There was a problem hiding this comment.
Perfect test case! It doesn't belong under the "Unrecognized types" block though. You can just put it at the very top (not under another describe), or add a describe called somethign like "runtimeOptions validation"
Description
See the test changes (and PR comments) in this PR: [BREAKING IN 2.20] Added assert for compression ON grouping OFF by MarioJGMsoft · Pull Request #23608 · microsoft/FluidFramework
There were some tests that had compression enabled but grouping disabled. We had to choose to either enable grouping or disable compression in that PR. We should revisit each case and consider if the other should be added as well.
Reviewer Guidance
Let me know if you think there's anything else that I'm missing or that should be tested.
Fixes: AB#28720