Skip to content

Commit bd58217

Browse files
committed
Share --json envelope readers in CLI tests
About 25 test files each carried a private copy of the same --json envelope readers: parse stdout, read events[], find an event, and collect error and warning codes. The copies had drifted on strictness and shape, so the next suite to assert on codes would add one more. tests/common/envelope.rs now owns them, re-exported from common and includable on its own. The migrated files delete their copies, and a ratchet in the cli binary keeps new ones out (files changed by open PRs stay pending). Its self-tests run each former shape through the shared readers on the inputs where the copies differed. No production change. Assisted-by: Claude Code:claude-opus-5-5
1 parent 5f9c320 commit bd58217

28 files changed

Lines changed: 573 additions & 460 deletions

‎crates/socket-patch-cli/tests/apply/check_verifies_installed_tree.rs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ use std::path::Path;
1212
use serde_json::{json, Value};
1313

1414
use crate::common;
15+
use common::envelope::events;
1516
use common::{git_sha256, parse_json_envelope, run_with_env};
1617

1718
const PURL: &str = "pkg:npm/check-target@1.0.0";
@@ -66,10 +67,6 @@ fn project(installed: Option<&[u8]>) -> tempfile::TempDir {
6667
tmp
6768
}
6869

69-
fn events(env: &Value) -> Vec<Value> {
70-
env["events"].as_array().cloned().unwrap_or_default()
71-
}
72-
7370
/// The regression: an unpatched installed copy is drift (exit 1), in both
7471
/// output modes, and `--check` writes nothing.
7572
#[test]
Lines changed: 268 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,268 @@
1+
//! One set of `--json` envelope readers for every CLI test target (#1089).
2+
//!
3+
//! `common/envelope.rs` owns parsing stdout and reading `events[]`, their
4+
//! `errorCode`s and the `warnings[]` codes. The self-tests below run every
5+
//! shape the per-file copies took through it, on the inputs where those
6+
//! copies differed; the ratchet keeps new private copies out.
7+
8+
use std::path::Path;
9+
10+
use serde_json::{json, Value};
11+
12+
use crate::common::envelope::{
13+
all_codes, codes_in, event_codes, event_triples, events, find_event, parse_json_envelope,
14+
};
15+
16+
/// Files that still define a private envelope reader. Each is changed by an
17+
/// open PR, is a shared module whose includers are, or keeps a rule of its
18+
/// own (a stricter or wider read). Migrate a file onto `common::envelope`
19+
/// and delete its entry; a stale entry is not an error, so a PR that
20+
/// migrates one never turns another red.
21+
const PENDING_PRIVATE_READERS: &[&str] = &[
22+
"covgap_commands_rollback.rs",
23+
"e2e_redirect_gradle_build.rs",
24+
"e2e_redirect_pnpm_build.rs",
25+
"e2e_socket_yml_policy.rs",
26+
"e2e_vendor_composer_build.rs",
27+
"e2e_vendor_npm_build.rs",
28+
"e2e_vendor_pnpm_build.rs",
29+
"e2e_vendor_pypi_build.rs",
30+
"e2e_vendor_vlt_build.rs",
31+
"e2e_vendor_yarn_berry_build.rs",
32+
"hosted_memory_engine.rs",
33+
"in_process_redirect.rs",
34+
"in_process_rollback_hosted.rs",
35+
"in_process_vendor.rs",
36+
"in_process_vendor_bun_takeover.rs",
37+
"in_process_vendor_npm_v1_takeover.rs",
38+
"in_process_vendor_pnpm_takeover.rs",
39+
"vendor_eject_bun_lockb.rs",
40+
"vendor_jvm_cli.rs",
41+
"e2e_vex_lockfile/cargo.rs",
42+
"in_process_vendor_bun_takeover/vlt.rs",
43+
"remove/covgap_commands_remove.rs",
44+
"scan/scan_paths_e2e.rs",
45+
"vlt_hosted_common/mod.rs",
46+
"vlt_vendor_common/mod.rs",
47+
];
48+
49+
/// The top-level helper names the per-file copies used.
50+
const READER_NAMES: &[&str] = &[
51+
"events",
52+
"find_event",
53+
"parse_envelope",
54+
"parse_json_envelope",
55+
"event_codes",
56+
"event_triples",
57+
"warning_codes",
58+
"codes",
59+
"all_codes",
60+
"codes_in",
61+
];
62+
63+
/// Every `.rs` file under `tests/`, as (`/`-joined relative path, text).
64+
fn test_sources() -> Vec<(String, String)> {
65+
fn walk(dir: &Path, root: &Path, out: &mut Vec<(String, String)>) {
66+
let mut entries: Vec<_> = std::fs::read_dir(dir)
67+
.unwrap_or_else(|e| panic!("read {}: {e}", dir.display()))
68+
.map(|e| e.unwrap().path())
69+
.collect();
70+
entries.sort();
71+
for path in entries {
72+
if path.is_dir() {
73+
walk(&path, root, out);
74+
} else if path.extension().is_some_and(|e| e == "rs") {
75+
let rel = path
76+
.strip_prefix(root)
77+
.unwrap()
78+
.components()
79+
.map(|c| c.as_os_str().to_string_lossy().into_owned())
80+
.collect::<Vec<_>>()
81+
.join("/");
82+
let text = std::fs::read_to_string(&path)
83+
.unwrap_or_else(|e| panic!("read {}: {e}", path.display()));
84+
out.push((rel, text));
85+
}
86+
}
87+
}
88+
let root = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests");
89+
let mut out = Vec::new();
90+
walk(&root, &root, &mut out);
91+
out
92+
}
93+
94+
/// Whether `text` defines a top-level envelope reader of its own.
95+
fn defines_private_reader(text: &str) -> bool {
96+
text.lines().any(|line| {
97+
let line = line
98+
.strip_prefix("pub(crate) ")
99+
.or_else(|| line.strip_prefix("pub "))
100+
.unwrap_or(line);
101+
line.strip_prefix("fn ").is_some_and(|rest| {
102+
READER_NAMES.iter().any(|name| {
103+
rest.strip_prefix(name)
104+
.is_some_and(|after| after.starts_with('(') || after.starts_with('<'))
105+
})
106+
})
107+
})
108+
}
109+
110+
#[test]
111+
fn no_new_private_envelope_readers() {
112+
let unexpected: Vec<String> = test_sources()
113+
.into_iter()
114+
.filter(|(rel, text)| {
115+
!rel.starts_with("common/")
116+
&& !PENDING_PRIVATE_READERS.contains(&rel.as_str())
117+
&& defines_private_reader(text)
118+
})
119+
.map(|(rel, _)| rel)
120+
.collect();
121+
assert!(
122+
unexpected.is_empty(),
123+
"these test files define their own --json envelope reader: {unexpected:?}. \
124+
Use `common::envelope` instead (`use common::envelope::..`, or \
125+
`#[path = \"common/envelope.rs\"] mod envelope;` in a file without \
126+
`mod common`). Do not add them to PENDING_PRIVATE_READERS."
127+
);
128+
}
129+
130+
#[test]
131+
fn the_detector_sees_every_former_copy_shape() {
132+
for copy in [
133+
"fn parse_envelope(stdout: &str) -> serde_json::Value {",
134+
"fn events(envelope: &Value) -> &Vec<Value> {",
135+
"fn events(stdout: &str) -> Vec<(String, String, String)> {",
136+
"fn find_event<'a>(env: &'a Value, action: &str, code: Option<&str>) -> &'a Value {",
137+
"fn find_event<'a>(",
138+
"pub fn codes(env: &Value) -> Vec<String> {",
139+
"fn warning_codes(doc: &Value) -> Vec<&str> {",
140+
"fn event_codes(v: &Value) -> Vec<String> {",
141+
] {
142+
assert!(defines_private_reader(copy), "missed: {copy}");
143+
}
144+
for not_a_copy in [
145+
"use common::envelope::{events, find_event};",
146+
" fn warning_codes(&self) -> Vec<String> {",
147+
"fn events_for_purl(env: &Value) -> Vec<Value> {",
148+
"fn codes_of_lock(lock: &str) -> Vec<String> {",
149+
"let codes = codes_in(&env[\"warnings\"]);",
150+
] {
151+
assert!(
152+
!defines_private_reader(not_a_copy),
153+
"false positive: {not_a_copy}"
154+
);
155+
}
156+
}
157+
158+
fn sample() -> Value {
159+
json!({
160+
"status": "partial_failure",
161+
"events": [
162+
{"action": "vendored", "purl": "pkg:npm/a@1.0.0"},
163+
{"action": "failed", "purl": "pkg:npm/b@1.0.0", "errorCode": "vendor_lock_drift"},
164+
{"action": "skipped", "errorCode": "vendored"},
165+
],
166+
"warnings": [{"code": "w_top"}, {"code": 7}, {"message": "no code"}],
167+
"redirect": {"warnings": [{"code": "w_redirect"}]},
168+
"vex": {"warnings": [{"code": "w_vex", "errorCode": "e_vex"}]},
169+
})
170+
}
171+
172+
#[test]
173+
fn parse_json_envelope_reads_padded_stdout_and_panics_on_text() {
174+
// `vendor_group_commit_e2e` trimmed stdout before parsing; serde
175+
// already skips the surrounding whitespace.
176+
assert_eq!(
177+
parse_json_envelope("\n {\"events\": []}\n")["events"],
178+
json!([])
179+
);
180+
let err = std::panic::catch_unwind(|| parse_json_envelope("error: no manifest")).unwrap_err();
181+
let msg = err.downcast_ref::<String>().unwrap();
182+
assert!(msg.contains("error: no manifest"), "{msg}");
183+
}
184+
185+
#[test]
186+
fn events_is_strict_and_event_codes_is_lenient() {
187+
let env = sample();
188+
assert_eq!(events(&env).len(), 3);
189+
assert_eq!(event_codes(&env), ["vendor_lock_drift", "vendored"]);
190+
// The lenient copies (`mode_migration_bun`, `vlt_vendor_common`) read a
191+
// nested `vendor` envelope or none at all as "no codes".
192+
assert!(event_codes(&json!({"status": "error"})).is_empty());
193+
assert!(std::panic::catch_unwind(|| events(&json!({"status": "error"})).len()).is_err());
194+
}
195+
196+
#[test]
197+
fn find_event_matches_action_and_optional_code() {
198+
let env = sample();
199+
assert_eq!(
200+
find_event(&env, "vendored", None)["purl"],
201+
"pkg:npm/a@1.0.0"
202+
);
203+
assert_eq!(
204+
find_event(&env, "failed", Some("vendor_lock_drift"))["purl"],
205+
"pkg:npm/b@1.0.0"
206+
);
207+
// The golang copy required a code and returned `None`; a miss now panics
208+
// with the envelope.
209+
let env2 = env.clone();
210+
let err = std::panic::catch_unwind(move || {
211+
find_event(&env2, "failed", Some("other")).clone();
212+
})
213+
.unwrap_err();
214+
let msg = err.downcast_ref::<String>().unwrap();
215+
assert!(
216+
msg.contains("vendor_lock_drift"),
217+
"panic shows the envelope: {msg}"
218+
);
219+
}
220+
221+
#[test]
222+
fn event_triples_fill_absent_fields_with_empty_strings() {
223+
assert_eq!(
224+
event_triples(&sample()),
225+
[
226+
("pkg:npm/a@1.0.0".into(), "vendored".into(), String::new()),
227+
(
228+
"pkg:npm/b@1.0.0".into(),
229+
"failed".into(),
230+
"vendor_lock_drift".into()
231+
),
232+
(String::new(), "skipped".into(), "vendored".into()),
233+
]
234+
);
235+
}
236+
237+
#[test]
238+
fn codes_in_reads_any_warnings_array_and_skips_non_strings() {
239+
let env = sample();
240+
assert_eq!(codes_in(&env["warnings"]), ["w_top"]);
241+
assert_eq!(codes_in(&env["redirect"]["warnings"]), ["w_redirect"]);
242+
// Missing arrays read as empty, as every former copy did.
243+
assert!(codes_in(&env["nope"]["warnings"]).is_empty());
244+
// The sbt agent and gradle agent copies chained two arrays.
245+
let both: Vec<String> = [&env["warnings"], &env["vex"]["warnings"]]
246+
.into_iter()
247+
.flat_map(codes_in)
248+
.collect();
249+
assert_eq!(both, ["w_top", "w_vex"]);
250+
}
251+
252+
#[test]
253+
fn all_codes_collects_code_and_error_code_at_any_depth() {
254+
let codes = all_codes(&sample());
255+
// `mode_migration_vlt` pushed `errorCode` before `code` per object; the
256+
// callers only ask membership, so order is not part of the contract.
257+
for want in [
258+
"vendor_lock_drift",
259+
"vendored",
260+
"w_top",
261+
"w_redirect",
262+
"w_vex",
263+
"e_vex",
264+
] {
265+
assert!(codes.iter().any(|c| c == want), "{want} in {codes:?}");
266+
}
267+
assert_eq!(codes.len(), 6, "non-string codes are skipped: {codes:?}");
268+
}

‎crates/socket-patch-cli/tests/cli/main.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ mod cli_dry_run_paths_e2e;
1616
mod covgap_api_client;
1717
mod covgap_commands_list;
1818
mod covgap_output;
19+
mod envelope_helper_copies;
1920
mod interactive_prompts_e2e;
2021
mod output_modes_e2e;
2122
mod shared_helper_copies;

0 commit comments

Comments
 (0)