Skip to content

Commit 5df7d2a

Browse files
committed
test(gl): pin the local-add print selection, not just its helper
local_add_refusal had six unit tests and the wiring had none, so replacing the whole match at the call site with an unconditional "Added to local peer list." left every test green. That is the shape the store already names: a helper test plus a status-mapping test does not prove the call site. Extract the selection into local_add_report, following the same print-selection-helper precedent used for the certificate DID hint, and assert both directions: a refusal never yields the success line, and a 200 never yields the warning. Collapsing the match to always return the success line now turns that red. The call site is left as a two-line dispatch carrying no logic.
1 parent 8313c10 commit 5df7d2a

1 file changed

Lines changed: 58 additions & 4 deletions

File tree

‎crates/gl/src/peer.rs‎

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,21 @@ fn remote_announce_failure(status: reqwest::StatusCode, raw: &str) -> Option<Str
136136
Some(format!("announce failed ({status}): {msg}"))
137137
}
138138

139+
/// Which line the local peer-add prints, and whether it is a warning.
140+
///
141+
/// Split out because the SELECTION is what the fix is: `local_add_refusal` was
142+
/// already unit-tested six ways, and replacing this whole match with an
143+
/// unconditional "Added to local peer list." still left 312 tests green. The
144+
/// helper was pinned; the wiring was not, which is the shape
145+
/// `unit-test-on-helper-does-not-prove-handler-wiring` names. With the choice
146+
/// extracted, the call site is a two-line dispatch carrying no logic.
147+
fn local_add_report(status: reqwest::StatusCode, body: &Value) -> (bool, String) {
148+
match local_add_refusal(status, body) {
149+
Some(warning) => (true, warning),
150+
None => (false, " Added to local peer list.".to_string()),
151+
}
152+
}
153+
139154
/// The block printed after a peer accepts our announce. Split out so the
140155
/// defanging is assertable: the DID and URL are the remote peer's own strings,
141156
/// and `peer_url` is caller-chosen, so this is the least trusted body the
@@ -224,9 +239,11 @@ async fn cmd_add(peer_url: String, node: String, dir: Option<PathBuf>) -> Result
224239
// routes a non-success status to the warning.
225240
let raw = read_body_capped(resp, 8 * 1024).await;
226241
let result: Value = serde_json::from_str(&raw).unwrap_or(Value::Null);
227-
match local_add_refusal(status, &result) {
228-
Some(warning) => eprintln!("{warning}"),
229-
None => println!(" Added to local peer list."),
242+
let (is_warning, line) = local_add_report(status, &result);
243+
if is_warning {
244+
eprintln!("{line}");
245+
} else {
246+
println!("{line}");
230247
}
231248
}
232249
Err(e) => eprintln!("warning: local peer list not updated: {e}"),
@@ -291,7 +308,10 @@ async fn cmd_resolve(did: String, node: String) -> Result<()> {
291308

292309
#[cfg(test)]
293310
mod tests {
294-
use super::{announced_peer_summary, cmd_add, local_add_refusal, remote_announce_failure};
311+
use super::{
312+
announced_peer_summary, cmd_add, local_add_refusal, local_add_report,
313+
remote_announce_failure,
314+
};
295315
use reqwest::StatusCode;
296316
use serde_json::json;
297317

@@ -508,6 +528,40 @@ mod tests {
508528
assert!(remote_announce_failure(StatusCode::OK, r#"{"peer_count":3}"#).is_none());
509529
}
510530

531+
/// The selection, not the helper. A refusal must never yield the success
532+
/// line, and the success line must never carry the warning prefix. This is
533+
/// what stayed green when the entire match was replaced with an
534+
/// unconditional "Added to local peer list."
535+
#[test]
536+
fn a_local_add_refusal_never_selects_the_success_line() {
537+
let (is_warning, line) = local_add_report(
538+
StatusCode::FORBIDDEN,
539+
&json!({ "message": "unproven announce cannot change an existing peer's http_url: did:key:zAbc" }),
540+
);
541+
542+
assert!(is_warning, "a 403 must select the warning stream: {line:?}");
543+
assert!(
544+
!line.contains("Added to local peer list"),
545+
"a refusal rendered as the success line: {line:?}"
546+
);
547+
assert!(
548+
line.contains("unproven announce cannot change an existing peer"),
549+
"the node's reason must reach the user: {line:?}"
550+
);
551+
}
552+
553+
/// The other direction, so the fix cannot be satisfied by always warning.
554+
#[test]
555+
fn an_accepted_local_add_selects_the_success_line() {
556+
let (is_warning, line) = local_add_report(StatusCode::OK, &json!({ "peer_count": 3 }));
557+
558+
assert!(!is_warning, "a 200 must not select the warning stream");
559+
assert!(
560+
line.contains("Added to local peer list"),
561+
"the success line must still print: {line:?}"
562+
);
563+
}
564+
511565
/// The accepted-announce block prints the remote peer's own DID and URL, so
512566
/// a hostile peer answering 200 must not get escapes onto the terminal that
513567
/// way either. The refusal path is not the only sink in this command.

0 commit comments

Comments
 (0)