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)
| 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".
There was a problem hiding this comment.
Also, I think maybe the function wants to be called assertValidVersionForCollab or something along those lines?
Good call, will switch the name
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.
The reason I added a function was to avoid casting as MinimumVersionForCollab. The check I added in assertValidMinVersionForCollab doesn't really promise that the version inputted is type MinimumVersionForCollab (just that it's valid semver). IMO it will be fine since all the versions coming in are controlled from our version generation tools, but I thought it would be nice to have assertValidMinVersionForCollab setup so if we wanted to add more validation/guardrails in the future.
There was a problem hiding this comment.
Yeah, having the typeguard seems good to me as well, my point is about using assert from core-utils vs the native one from node (or simply an if (<is null>) { throw new Error("version must be valid semver")}. With the current assert, the error message will be replaced with a hex code during assert tagging for the next release, and in the scenarios where this function gets called (our pipelines, and I believe that's it?) I think outputting a hex code instead of the string directly is a con with no benefit.
There was a problem hiding this comment.
Ah I see what you meant now and I agree. Updated in latest
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