diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e58b32b..231523cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog +## Unreleased + +### Fixes + +#### `check -o json` and `-o csv` list each strict violation once + +A strict violation that fails the run is both a new violation and a strict-mode +violation, and the JSON and CSV formats listed it once for each, as two +identical entries. JSON also counted both in `violation_count`, so it could +exceed the number of distinct violations. Each violation now appears once, and +`violation_count` counts distinct violations. Text output is unchanged. + ## 0.5.0 ### Breaking Changes diff --git a/src/packs/csv.rs b/src/packs/csv.rs index 5193141e..dcb9797e 100644 --- a/src/packs/csv.rs +++ b/src/packs/csv.rs @@ -1,5 +1,3 @@ -use itertools::chain; - use super::checker::{ build_strict_violation_message, CheckAllResult, Violation, }; @@ -40,10 +38,9 @@ pub fn write_csv( if !&result.reportable_violations.is_empty() || !&result.strict_mode_violations.is_empty() { - let all = chain!( - &result.reportable_violations, - &result.strict_mode_violations - ); + let all = result + .reportable_violations + .union(&result.strict_mode_violations); for violation in all { let identifier = &violation.identifier; diff --git a/src/packs/json.rs b/src/packs/json.rs index 228dfbc3..0483ec71 100644 --- a/src/packs/json.rs +++ b/src/packs/json.rs @@ -3,7 +3,6 @@ //! Serializes check results (violations, stale TODOs, and summary) to JSON. //! See `schema/check-output.json` for the JSON Schema specification. -use itertools::chain; use serde::Serialize; use super::checker::{ @@ -71,10 +70,9 @@ pub fn write_json( config: &Configuration, writer: W, ) -> anyhow::Result<()> { - let all_violations = chain!( - &result.reportable_violations, - &result.strict_mode_violations - ); + let all_violations = result + .reportable_violations + .union(&result.strict_mode_violations); // JSON outputs raw structured data - consumers can format as needed. // Location is provided as separate file/line/column fields. diff --git a/tests/check_test.rs b/tests/check_test.rs index 7172ba26..41805c8a 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -494,10 +494,10 @@ fn test_check_with_strict_mode_output_csv() -> Result<(), Box> { // so there is nothing left to assert against in the CSV. Note that this // fixture ships no `package_todo.yml`, so the violation is an ordinary // unrecorded one: this test does not exercise strict tolerance and passes - // with the recorded filter disabled. The duplicate assertion it used to - // carry was byte-identical to the one below it, so dropping it costs no - // coverage. - cargo_bin_cmd!("pks") + // with the recorded filter disabled. It compares whole lines because an + // unrecorded strict violation is in both `reportable_violations` and + // `strict_mode_violations`, and must still get exactly one row. + let output = cargo_bin_cmd!("pks") .arg("--project-root") .arg("tests/fixtures/contains_strict_violations") .arg("check") @@ -505,10 +505,127 @@ fn test_check_with_strict_mode_output_csv() -> Result<(), Box> { .arg("csv") .assert() .code(1) - .stdout(predicate::str::contains("Violation,Strict?,File,Constant,Referencing Pack,Defining Pack,Message")) - .stdout(predicate::str::contains( - "privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack\'s package.yml file", - )); + .get_output() + .stdout + .clone(); + + let stdout = String::from_utf8(output)?; + assert_eq!( + stdout.lines().collect::>(), + [ + "Violation,Strict?,File,Constant,Referencing Pack,Defining Pack,Message", + "privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", + ] + ); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_strict_mode_output_json() -> Result<(), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/contains_strict_violations") + .arg("check") + .arg("-o") + .arg("json") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!(json_output["summary"]["violation_count"], 1); + assert_eq!(json_output["summary"]["stale_todo_count"], 0); + assert_eq!(json_output["summary"]["strict_violation_count"], 1); + assert_eq!(json_output["summary"]["success"], false); + + let violations = json_output["violations"].as_array().unwrap(); + assert_eq!(violations.len(), 1); + assert_eq!(violations[0]["violation_type"], "privacy"); + assert_eq!(violations[0]["constant_name"], "::Bar"); + assert_eq!(violations[0]["strict"], true); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_recorded_strict_mode_violation_ignoring_todo_csv( +) -> Result<(), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("--ignore-recorded-violations") + .arg("-o") + .arg("csv") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let stdout = String::from_utf8(output)?; + let mut lines = stdout.lines(); + assert_eq!( + lines.next(), + Some("Violation,Strict?,File,Constant,Referencing Pack,Defining Pack,Message") + ); + let mut rows: Vec<&str> = lines.collect(); + rows.sort(); + assert_eq!( + rows, + [ + "dependency,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have dependency violations on packs/bar because strict mode is enabled for dependency violations in the enforcing pack's package.yml file", + "privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", + ] + ); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_recorded_strict_mode_violation_ignoring_todo_json( +) -> Result<(), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("--ignore-recorded-violations") + .arg("-o") + .arg("json") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!(json_output["summary"]["violation_count"], 2); + assert_eq!(json_output["summary"]["stale_todo_count"], 0); + assert_eq!(json_output["summary"]["strict_violation_count"], 2); + assert_eq!(json_output["summary"]["success"], false); + + let violations = json_output["violations"].as_array().unwrap(); + let mut violation_types: Vec<&str> = violations + .iter() + .map(|v| v["violation_type"].as_str().unwrap()) + .collect(); + violation_types.sort(); + assert_eq!(violation_types, ["dependency", "privacy"]); + assert!(violations.iter().all(|v| v["strict"] == true)); common::teardown(); Ok(())