Add version comparison support to Microsoft/OSInfo#1603
Open
SteveL-MSFT wants to merge 11 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. |
SteveL-MSFT
force-pushed
the
os-ver-compare
branch
5 times, most recently
from
3aa1d92 to
5da1477
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>
SteveL-MSFT
force-pushed
the
os-ver-compare
branch
from
dcb4141 to
7dad84b
Compare
Member
Author
|
@michaeltlombardi should be one struct now |
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