Add version comparison support to Microsoft/OSInfo#1603
Open
SteveL-MSFT wants to merge 10 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a native test implementation for the Microsoft/OSInfo DSC resource and extends the version property to support comparison operators during test, enabling minimum/maximum OS version assertions (per #1521).
Changes:
- Added a dedicated
testoperation to theMicrosoft/OSInforesource manifest and a correspondingosinfo testCLI subcommand. - Implemented version constraint parsing/comparison (
>,<,=,>=,<=) for the resource’s test operation viaversion-compare. - Added Pester coverage for version operator behavior in the resource test flow.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| resources/osinfo/tests/osinfo.tests.ps1 | Adds Pester tests validating version operator constraints via dsc resource test. |
| resources/osinfo/src/main.rs | Adds test subcommand handling and refactors CLI arg dispatch. |
| resources/osinfo/osinfo.dsc.resource.json | Declares an explicit test capability and documents operator-prefixed version constraints; adds _inDesiredState to schema. |
| lib/dsc-lib-osinfo/src/lib.rs | Implements perform_test, version constraint parsing, and _inDesiredState emission. |
| lib/dsc-lib-osinfo/Cargo.toml | Adds version-compare dependency for OSInfo test/version comparison. |
| Cargo.toml | Adds version-compare to workspace dependencies. |
| Cargo.lock | Locks version-compare dependency version. |
| /// with an ASCII digit. Strings like `">> 1.0"` (double operator) are | ||
| /// therefore treated as literal exact-match strings rather than producing a | ||
| /// misleading parsed operator. | ||
| fn parse_version_constraint(constraint: &str) -> (&str, &str) { |
Collaborator
There was a problem hiding this comment.
Not blocking for this PR but a few general notes on the new functionality:
- In the current implementation, the
versionproperty only gets an autogenerated JSON Schema astype: [string, null]- adding version requirement syntax can't be represented through the schema without either providing the schema directly (schema_with) or asschemarsattributes. - In this implementation, we're parsing and validating the string on demand and only in the
testoperation. - This is an entirely different version requirement syntax from the
SemanticVersionReqindsc-libwhich is okay but worth noting.
We could encapsulate this in a couple of type definitions, which would enable better documentation and validation.
Member
Author
There was a problem hiding this comment.
In this case, I don't believe Linux, macOS, or Windows follows semver, hence not trying to convert them to semver
3c12edb to
3aa1d92
Compare
Build-RustProject now uses `cargo llvm-cov build --no-report` when -CodeCoverage is specified, producing instrumented binaries that write profraw data when executed. Previously the -CodeCoverage parameter was accepted but unused — the function always called `cargo build`. When Pester tests run with -CodeCoverage enabled, LLVM_PROFILE_FILE is set via a new Get-LlvmProfileFilePattern helper so that externally invoked instrumented binaries write profraw data to the target directory where `cargo llvm-cov report` can discover it. The CI workflow is updated to combine the build and coverage test steps into a single `./build.ps1 -Clippy -Test -CodeCoverage` invocation, ensuring the instrumented build is used for coverage collection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cargo-llvm-cov does not have a `build` subcommand. Instead, use `cargo llvm-cov show-env` to retrieve the environment variables (RUSTC_WRAPPER, LLVM_PROFILE_FILE, etc.) and set them in the process. A normal `cargo build` then produces instrumented binaries transparently. Replace Get-LlvmProfileFilePattern with Set-LlvmCovEnvironment / Reset-LlvmCovEnvironment which set and restore all required env vars. The env vars are set before the build so both cargo build and Pester tests benefit from instrumentation, and restored after the LCOV report is generated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Since Set-LlvmCovEnvironment sets RUSTC_WRAPPER and LLVM_PROFILE_FILE, normal `cargo build` and `cargo test` already produce instrumented binaries and write profraw data. Calling `cargo llvm-cov test` on top of the pre-set env vars caused conflicts and errors. Remove the -CodeCoverage parameter from both Build-RustProject and Test-RustProject — the env vars make instrumentation transparent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a step to the coverage-report job that exits with an error when the coverage percentage on changed Rust code is less than 70%. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add tests covering edition (Windows-only), codename (Linux-only), bitness, and architecture properties in the OSInfo test operation. Also tests combining multiple properties to verify partial mismatch detection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The coverage report showed 0% because the build step excluded Pester tests (-ExcludePesterTests). Since the osinfo test operation code paths are only exercised by Pester tests (not Rust unit tests), the instrumented binaries never recorded any hits for those functions. Change the build step to run the 'resources' Pester test group alongside Rust tests so the instrumented osinfo binary is exercised and profraw data is recorded. Also filter out CARGO_LLVM_COV_SHOW_ENV from Set-LlvmCovEnvironment since propagating that flag causes cargo-llvm-cov to print env vars and exit rather than performing compilation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The coverage report used github.event.pull_request.base.sha directly with three-dot git diff syntax. After a rebase or when the base branch advances, base.sha points to the current tip of the base branch rather than the common ancestor, causing the diff to include unrelated changes or miss actual PR changes. Fix by computing git merge-base between base and head SHAs in the coverage-report job, then using two-dot diff syntax throughout. This ensures only the PR's own changes are analyzed regardless of rebases or upstream merges. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The coverage-report job failed to match LCOV source file paths because the matching logic did not handle relative paths from cargo-llvm-cov. Added direct comparison with relative file path and normalized slash handling. Also added verbose diagnostic output when no match is found to aid future debugging. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3aa1d92 to
5da1477
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
testoperation instead of relying on synthetic testversionto have comparison operators:>,<,=,<=,>=(whitespace is allowed)osinfoCLI with newtestsubcommand and refactored the arg parsing in main.rsExample for this PR on fixed CC report:
PR Context
Fix #1521