🛡️ Sentry: [test coverage improvement]#1099
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds comprehensive unit tests to src/artifact.rs covering schema versioning, artifact classification, compatibility classes, and serialization, as well as adding a clippy allow attribute to src/env/docker.rs. The review feedback suggests improving the idiomatic quality of the tests by replacing verbose match statements with assert!(matches!(...)) macros and recommends adding test coverage for MalformedHeader error cases.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ArtifactSchemaError::KindMismatch { | ||
| expected, found, .. | ||
| } => { | ||
| assert_eq!(expected, ArtifactKind::SweepResults); | ||
| assert_eq!(found, ArtifactKind::Trajectory); | ||
| } | ||
| _ => panic!("Expected KindMismatch"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Using a verbose match statement with a manual panic! is less idiomatic in Rust. We can simplify this assertion using assert!(matches!(...)) to improve readability and maintainability.
assert!(matches!(
err,
ArtifactSchemaError::KindMismatch {
expected: ArtifactKind::SweepResults,
found: ArtifactKind::Trajectory,
..
}
));| ArtifactSchemaError::UnsupportedFuture { | ||
| version, | ||
| supported_major, | ||
| .. | ||
| } => { | ||
| assert_eq!(version.major, 99); | ||
| assert_eq!(supported_major, 1); | ||
| } | ||
| _ => panic!("Expected UnsupportedFuture"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Similarly, we can simplify this assertion using assert!(matches!(...)) instead of a verbose match block with a manual panic!.
assert!(matches!(
err,
ArtifactSchemaError::UnsupportedFuture {
version: ArtifactSchemaVersion { major: 99, .. },
supported_major: 1,
..
}
));| assert_eq!(res.warnings.len(), 1); | ||
| assert!(res.warnings[0].contains("newer additive minor")); | ||
| } | ||
|
|
There was a problem hiding this comment.
The PR aims to improve test coverage for src/artifact.rs, but it currently lacks test coverage for the MalformedHeader error cases in classify_json_value. Adding a test case to cover these scenarios ensures that malformed headers are handled correctly and prevents regressions.
}
#[test]
fn classify_malformed_header() {
// Case 1: Only one of artifact_kind or schema_version is present
let val1 = json!({
"artifact_kind": "trajectory"
});
let err1 = classify_json_value(&val1, ArtifactKind::Trajectory, "path").unwrap_err();
assert!(matches!(err1, ArtifactSchemaError::MalformedHeader { .. }));
// Case 2: Invalid artifact_kind
let val2 = json!({
"artifact_kind": "invalid_kind",
"schema_version": {"major": 1, "minor": 13}
});
let err2 = classify_json_value(&val2, ArtifactKind::Trajectory, "path").unwrap_err();
assert!(matches!(err2, ArtifactSchemaError::MalformedHeader { .. }));
// Case 3: Invalid schema_version
let val3 = json!({
"artifact_kind": "trajectory",
"schema_version": "invalid_version"
});
let err3 = classify_json_value(&val3, ArtifactKind::Trajectory, "path").unwrap_err();
assert!(matches!(err3, ArtifactSchemaError::MalformedHeader { .. }));
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #1099 +/- ##
==========================================
+ Coverage 86.63% 86.66% +0.02%
==========================================
Files 139 139
Lines 86826 86938 +112
==========================================
+ Hits 75220 75341 +121
+ Misses 11606 11597 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
🎯 Target:
src/artifact.rsmissing test coverage andsrc/env/docker.rstest lints.💣 Risk:
artifact.rscontains critical data types (ArtifactSchemaVersion,ArtifactKind,ArtifactHeader,ArtifactCompatibility,ArtifactSchemaError) and functions (classify_json_value,to_string_pretty,to_writer_pretty) for serialization/deserialization and checking artifacts. A failure here could corrupt metrics, prevent loading results, or panic during artifact reads.🧪 Strategy: Added a comprehensive
testsmodule directly insidesrc/artifact.rs(using#![allow(clippy::unwrap_used)]) to test schema version logic, artifact kinds and headers, classification/deserialization logic (including errors like version mismatches and unsupported futures), and serialization to string/writer. Also fixed an existingunwrap_usedlint violation insidesrc/env/docker.rs's test module.🔬 Verification:
cargo test artifactandcargo test env::docker --features dockerpassing.cargo clippy --all-targets --all-features -- -D warningspassing cleanly.PR created automatically by Jules for task 2431047419031674089 started by @madmax983