improvement(test-version-utils): Automatically set minVersionForCollab in e2e test generation #24809
Conversation
github-actions
bot
added
area: tests
scottn12
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR automates the selection of minVersionForCollab in E2E test setups by removing manual runtime option filtering and introducing a helper to derive the minimum version automatically.
- Replaced
filterRuntimeOptionsForVersionwithgetMinVersionForCollaband assertion helper. - Updated E2E tests to pass or default
minVersionForCollab. - Removed unused version-based filtering logic and related imports.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/test/test-version-utils/src/compatUtils.ts | Added getMinVersionForCollab, removed filterRuntimeOptionsForVersion, updated factory calls. |
| packages/test/test-end-to-end-tests/src/test/offline/stashedOps.spec.ts | Enabled enableRuntimeIdCompressor in test config. |
| packages/test/test-end-to-end-tests/src/test/containerRuntime.spec.ts | Added explicit minVersionForCollab in container runtime test. |
| packages/test/test-end-to-end-tests/src/test/compression.spec.ts | Extended setupContainers signature and passed minVersionForCollab. |
| packages/test/test-end-to-end-tests/src/test/blobs.spec.ts | Set default minVersionForCollab in blob container config. |
Comments suppressed due to low confidence (1)
packages/test/test-version-utils/src/compatUtils.ts:68
- The call to
semver.comparerequires thesemvermodule to be imported. Please addimport semver from 'semver';at the top of the file.
return semver.compare(containerRuntimeVersion, containerRuntimeForLoadingVersion) <= 0
| * If a version runtime supports some options, such options are enabled to increase a chance of | ||
| * hitting feature set controlled by such options, and thus increase chances of finding product bugs. | ||
| * Determines the minimumVersionForCollab that should be used for cross-client compatibility scenarios. | ||
| * We will use the lesser of the containerRuntimeVersion and containerRuntimeForLoadingVersion |
There was a problem hiding this comment.
It's not apparent with containerRuntimeForLoadingVersion is. I would explain the parameters more in the comment. Maybe even give an example to make it clear. These are complicated concepts so the easier we can make it to understand, the better.
There was a problem hiding this comment.
Makes sense, updated in latest
…sts (#24810) ## Description This PR updates e2e tests to prepare for the removal of `filterRuntimeOptionsForVersion()` in #24809. This PR is a subset of #24809 and only contains the changes to tests. The tests updated in this PR are updated for one of these two reasons: 1. Some runtime options that were automatically set by `filterRuntimeOptionsForVersion()` are not automatically set by `minVersionForCollab` . For example, `enableRuntimeIdCompressor` will never be enabled by default by any `minVersionForCollab`, but was enabled automatically by `filterRuntimeOptionsForVersion()`. In these cases, we need to manually enable `enableRuntimeIdCompressor` (if not already done by the test author). 2. Second, some tests that are enabled for `"FullCompat"` manually enable runtime options that collide the automatically assigned `minVersionForCollab`. For example, some tests in `compression.spec.ts` have batching/compression enabled. When these tests are running in cross-client compat mode, `minVersionForCollab` will be set as low as `1.4.0` (for the new client). This results in an error thrown unless the test explicitly sets the `minVersionForCollab` to a higher value. For more context on why we throw errors in this scenario, see #24625. ## Misc [AB#41365](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/41365)
| /** | ||
| * Asserts the given version is valid semver and is type MinimumVersionForCollab. | ||
| */ | ||
| function isMinimumVersionForCollab( |
There was a problem hiding this comment.
I found this function name a bit confusing. Before getting to its definition and its TSDoc, I thought it was supposed to return whether a particular version is a minimum of some kind, which didn't make sense since it was the only parameter and thus apparently wasn't being compared to something else.
Maybe the type should be just VersionForCollab, and the function isVersionForCollab?
| function isMinimumVersionForCollab( | ||
| version: string, | ||
| ): asserts version is MinimumVersionForCollab { | ||
| assert(semver.valid(version) !== null, "version must be valid semver"); |
There was a problem hiding this comment.
Would it be enough to use node's assert here instead of our custom one? I think this code doesn't end up in any production bundle so assert tagging doesn't matter.
Also, I think maybe the function wants to be called assertValidVersionForCollab or something along those lines? For an is<Something> function I think it's preferrable to always return true or false when we know that's actually the case (instead of only ever being able to return true or throwing). And the way we use this function seems to be more a "this should be true or bust", not really a "handle each scenario differently".
Description
This PR leverages
minVersionForCollabin e2e test generations by doing the following:minVersionForCollabfrom theITestContainerConfigobject.minVersionForCollabis provided inITestContainerConfig, then it will be set automatically. We will use the lesser of the container runtime version for creating/loading. (Note: If it's not a cross-client test, then the loading version will be undefined and we will use the creating version).These changes allow us to remove the
filterRuntimeOptionsForVersion()function.filterRuntimeOptionsForVersionwas used to set runtime options based on the container runtime verison, which is no longer necessary since that logic is automatically taken care of byminVersionForCollab.Misc
AB#41365