fix: feature-matrix CI failures for minimal and rvm test combos#708
Draft
anakrish wants to merge 1 commit into
Draft
fix: feature-matrix CI failures for minimal and rvm test combos#708anakrish wants to merge 1 commit into
anakrish wants to merge 1 commit into
Conversation
Collaborator
Author
|
@copilot review this PR using code-review skill and using deep-review skill |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes weekly tests/feature-matrix CI failures by properly skipping builtin-specific interpreter tests when features are disabled, and by making test_utils available to integration tests without requiring the yaml feature.
Changes:
- Added per-feature skip logic for interpreter YAML tests when
time,semver,glob,uuid, orregexare not enabled. - Introduced a
test-utilsfeature to exposetest_utilsto integration tests without depending onyaml. - Updated
test_utils::match_valuesto useserde_jsonoutput and updated the feature-matrix workflow to enabletest-utilsforrvmjobs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/tests/interpreter/mod.rs |
Adds feature-gated skip blocks so minimal feature sets don’t run builtin-specific tests. |
src/test_utils.rs |
Removes serde_yaml dependency in diff output by switching to serde_json. |
src/lib.rs |
Exposes test_utils under the new test-utils feature for integration tests. |
Cargo.toml |
Declares the new test-utils feature. |
.github/workflows/feature-matrix.yml |
Enables test-utils in relevant CI feature-matrix jobs. |
Comments suppressed due to low confidence (1)
src/tests/interpreter/mod.rs:1
- The skip checks use
file.contains(...), which can produce false positives (e.g., a directory name containing the substring) and can be brittle across path formats. Also,file.contains(\"regex/\")depends on/separators and may not match iffilecan contain platform separators or normalized paths. Prefer matching onPathcomponents or using stricter checks likeends_withfor specific files andstarts_with(\"regex/\")on a normalized forward-slash path (or aPathprefix check).
// Copyright (c) Microsoft Corporation.
| #[cfg(feature = "azure_policy")] | ||
| pub mod target; | ||
| #[cfg(any(test, all(feature = "yaml", feature = "std")))] | ||
| #[cfg(any(test, feature = "test-utils", all(feature = "yaml", feature = "std")))] |
Comment on lines
+647
to
+650
| if file.contains("globmatch.yaml") { | ||
| std::println!("skipped {file} without glob feature."); | ||
| return Ok(()); | ||
| } |
Comment on lines
+664
to
+667
| if file.contains("regex/") { | ||
| std::println!("skipped {file} without regex feature."); | ||
| return Ok(()); | ||
| } |
Comment on lines
+614
to
+668
| #[cfg(not(feature = "time"))] | ||
| { | ||
| let skip = [ | ||
| "add_date.yaml", | ||
| "date.yaml", | ||
| "clock.yaml", | ||
| "diff.yaml", | ||
| "format.yaml", | ||
| "now_ns.yaml", | ||
| "parse_duration_ns.yaml", | ||
| "parse_ns.yaml", | ||
| "parse_rfc3339_ns.yaml", | ||
| "weekday.yaml", | ||
| ]; | ||
| for s in skip { | ||
| if file.contains(s) { | ||
| std::println!("skipped {file} without time feature."); | ||
| return Ok(()); | ||
| } | ||
| } | ||
| } | ||
| #[cfg(not(feature = "semver"))] | ||
| { | ||
| let skip = ["semver/compare.yaml", "semver/is_valid.yaml"]; | ||
| for s in skip { | ||
| if file.contains(s) { | ||
| std::println!("skipped {file} without semver feature."); | ||
| return Ok(()); | ||
| } | ||
| } | ||
| } | ||
| #[cfg(not(feature = "glob"))] | ||
| { | ||
| if file.contains("globmatch.yaml") { | ||
| std::println!("skipped {file} without glob feature."); | ||
| return Ok(()); | ||
| } | ||
| } | ||
| #[cfg(not(feature = "uuid"))] | ||
| { | ||
| let skip = ["uuid/generate.yaml", "uuid/parse.yaml"]; | ||
| for s in skip { | ||
| if file.contains(s) { | ||
| std::println!("skipped {file} without uuid feature."); | ||
| return Ok(()); | ||
| } | ||
| } | ||
| } | ||
| #[cfg(not(feature = "regex"))] | ||
| { | ||
| if file.contains("regex/") { | ||
| std::println!("skipped {file} without regex feature."); | ||
| return Ok(()); | ||
| } | ||
| } |
Comment on lines
93
to
+98
| /// Diff-friendly equality helper used by multiple YAML suites. | ||
| pub fn match_values(computed: &Value, expected: &Value) -> Result<()> { | ||
| if computed != expected { | ||
| let expected_yaml = serde_yaml::to_string(expected)?; | ||
| let computed_yaml = serde_yaml::to_string(computed)?; | ||
| bail!("expected:\n{}computed:\n{}", expected_yaml, computed_yaml); | ||
| let expected_str = serde_json::to_string_pretty(expected)?; | ||
| let computed_str = serde_json::to_string_pretty(computed)?; | ||
| bail!("expected:\n{expected_str}\ncomputed:\n{computed_str}"); |
- Add per-feature skip logic in interpreter tests for time, semver, glob, uuid, and regex builtins when those features are not enabled. - Introduce a zero-dep `test-utils` feature to expose the test_utils module to integration tests without requiring the `yaml` feature. - Replace serde_yaml usage in test_utils with serde_json (always available) so the module has no optional-dep requirement. - Update feature-matrix workflow to add test-utils to rvm-based jobs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
anakrish
force-pushed
the
fix-feature-matrix-ci
branch
from
dc840f0 to
6a8e0a3
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.
Fixes the failures in the
tests/feature-matrixweekly CI run (run #25244035824).Root Causes
1. Missing per-feature skip logic in interpreter tests ("minimal" job)
Tests for
time,semver,glob,uuid, andregexbuiltins ran even when those features weren't enabled. The existing skip logic only handled theno_stdcase.2.
test_utilsmodule unavailable to integration tests ("library", "observability", "cherry-picked builtins" jobs)test_utilswas gated behindcfg(any(test, all(feature = "yaml", feature = "std"))). Sincecfg(test)doesn't apply to the library when compiled for integration tests, it required theyamlproduct feature. Integration tests withrvmbut withoutyamlcouldn't access the module.Fix
#[cfg(not(feature = "..."))]skip blocks for time, semver, glob, uuid, and regex builtin tests.test-utilsfeature that exposestest_utilswithout requiringyaml.serde_yamlusage intest_utils::match_valueswithserde_json(always available) so the module has no optional-dep requirements.test-utilsto rvm-based jobs.