diff --git a/crates/irtt-cli/src/cmd/client/output.rs b/crates/irtt-cli/src/cmd/client/output.rs index 6a782b2..f7fdd24 100644 --- a/crates/irtt-cli/src/cmd/client/output.rs +++ b/crates/irtt-cli/src/cmd/client/output.rs @@ -27,15 +27,14 @@ impl OutputConfig { columns: Option<&str>, header: HeaderMode, verbose: bool, - multi_target: bool, ) -> Result { let uses_default_columns = columns .map(|columns| columns.trim() == "default") .unwrap_or(true); let default_table_rows = format == OutputFormat::Table && uses_default_columns; let columns = match columns { - Some(columns) => parse_columns(columns, format, verbose, multi_target)?, - None => default_columns(format, verbose, multi_target), + Some(columns) => parse_columns(columns, format, verbose)?, + None => default_columns(format, verbose), }; Ok(Self { @@ -723,12 +722,7 @@ impl CellValue { } } -fn parse_columns( - input: &str, - format: OutputFormat, - verbose: bool, - multi_target: bool, -) -> Result, String> { +fn parse_columns(input: &str, format: OutputFormat, verbose: bool) -> Result, String> { let trimmed = input.trim(); if trimmed.is_empty() { return Err("--columns requires at least one column".to_owned()); @@ -737,7 +731,7 @@ fn parse_columns( return Ok(ALL_COLUMNS.to_vec()); } if trimmed == "default" { - return Ok(default_columns(format, verbose, multi_target)); + return Ok(default_columns(format, verbose)); } let mut columns = Vec::new(); @@ -754,10 +748,11 @@ fn parse_columns( Ok(columns) } -fn default_columns(format: OutputFormat, verbose: bool, multi_target: bool) -> Vec { +fn default_columns(format: OutputFormat, verbose: bool) -> Vec { match format { OutputFormat::Table => { let mut columns = vec![ + Column::Target, Column::Event, Column::Seq, Column::Rtt, @@ -767,9 +762,6 @@ fn default_columns(format: OutputFormat, verbose: bool, multi_target: bool) -> V Column::ServerProcessing, Column::Message, ]; - if multi_target { - columns.insert(0, Column::Target); - } if verbose { columns.extend([ Column::Remote, @@ -784,17 +776,7 @@ fn default_columns(format: OutputFormat, verbose: bool, multi_target: bool) -> V } columns } - OutputFormat::Csv | OutputFormat::Tsv | OutputFormat::Jsonl => { - if multi_target { - ALL_COLUMNS.to_vec() - } else { - ALL_COLUMNS - .iter() - .copied() - .filter(|column| *column != Column::Target) - .collect() - } - } + OutputFormat::Csv | OutputFormat::Tsv | OutputFormat::Jsonl => ALL_COLUMNS.to_vec(), } } @@ -1209,12 +1191,8 @@ mod tests { time::Instant, }; - fn output_config( - format: OutputFormat, - columns: Option<&str>, - multi_target: bool, - ) -> OutputConfig { - OutputConfig::new(format, columns, HeaderMode::Auto, false, multi_target).unwrap() + fn output_config(format: OutputFormat, columns: Option<&str>) -> OutputConfig { + OutputConfig::new(format, columns, HeaderMode::Auto, false).unwrap() } fn remote() -> SocketAddr { @@ -1254,38 +1232,48 @@ mod tests { } #[test] - fn default_table_columns_include_target_only_for_multi_target() { - let single = output_config(OutputFormat::Table, None, false) - .render_header() - .unwrap(); - let multi = output_config(OutputFormat::Table, None, true) + fn default_table_columns_always_include_target_first() { + let header = output_config(OutputFormat::Table, None) .render_header() .unwrap(); - assert!(!single.split_whitespace().any(|column| column == "target")); - assert_eq!(multi.split_whitespace().next(), Some("target")); + assert_eq!(header.split_whitespace().next(), Some("target")); } #[test] - fn default_structured_columns_include_target_for_multi_target() { - let single = output_config(OutputFormat::Csv, None, false) - .render_header() - .unwrap(); - let multi = output_config(OutputFormat::Csv, None, true) - .render_header() - .unwrap(); + fn default_structured_columns_always_include_target_first() { + for format in [OutputFormat::Csv, OutputFormat::Tsv, OutputFormat::Jsonl] { + let columns = default_columns(format, false); + assert_eq!(columns.first(), Some(&Column::Target)); + assert_eq!(columns, ALL_COLUMNS); + } + } - assert!(!single.split(',').any(|column| column == "target")); - assert_eq!(multi.split(',').next(), Some("target")); + #[test] + fn columns_default_keyword_matches_omitted_columns() { + for format in [ + OutputFormat::Table, + OutputFormat::Csv, + OutputFormat::Tsv, + OutputFormat::Jsonl, + ] { + let omitted = output_config(format, None).render_header(); + let explicit = output_config(format, Some("default")).render_header(); + assert_eq!(omitted, explicit); + } + } + + #[test] + fn custom_columns_do_not_insert_target() { + let config = output_config(OutputFormat::Csv, Some("seq,effective_rtt_us")); + let header = config.render_header().unwrap(); + + assert!(!header.split(',').any(|column| column == "target")); } #[test] fn custom_target_column_renders_label() { - let config = output_config( - OutputFormat::Csv, - Some("target,seq,effective_rtt_us"), - false, - ); + let config = output_config(OutputFormat::Csv, Some("target,seq,effective_rtt_us")); let line = config .render_event(&reply(), Some("ams"), Some(&EventRenderStats::default())) .unwrap(); @@ -1296,4 +1284,24 @@ mod tests { ); assert_eq!(line, "ams,7,1200"); } + + #[test] + fn default_csv_renders_target_label_in_row() { + let config = output_config(OutputFormat::Csv, None); + let line = config + .render_event(&reply(), Some("ams"), Some(&EventRenderStats::default())) + .unwrap(); + + assert_eq!(line.split(',').next(), Some("ams")); + } + + #[test] + fn default_jsonl_renders_target_key_first() { + let config = output_config(OutputFormat::Jsonl, None); + let line = config + .render_event(&reply(), Some("ams"), Some(&EventRenderStats::default())) + .unwrap(); + + assert!(line.starts_with("{\"target\":\"ams\"")); + } } diff --git a/crates/irtt-cli/src/cmd/client/run.rs b/crates/irtt-cli/src/cmd/client/run.rs index 5afd8ab..75d82eb 100644 --- a/crates/irtt-cli/src/cmd/client/run.rs +++ b/crates/irtt-cli/src/cmd/client/run.rs @@ -59,7 +59,6 @@ pub fn run_stream( args.columns.as_deref(), args.header, args.verbose, - multi_target, ) .map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err))?; let continuous = args.is_continuous(); diff --git a/crates/irtt-cli/tests/client_multi_target.rs b/crates/irtt-cli/tests/client_multi_target.rs index 0f2cc93..8fec7d8 100644 --- a/crates/irtt-cli/tests/client_multi_target.rs +++ b/crates/irtt-cli/tests/client_multi_target.rs @@ -72,8 +72,9 @@ fn list_columns_succeeds_without_target() { } #[test] -fn single_target_default_table_omits_target_and_accepts_pacing() { +fn single_target_default_table_includes_target_and_accepts_pacing() { let server = InTreeServer::start(); + let target = server.addr.to_string(); let output = Command::new(env!("CARGO_BIN_EXE_irtt-cli")) .args([ @@ -85,7 +86,7 @@ fn single_target_default_table_omits_target_and_accepts_pacing() { "burst", "--header", "always", - &server.addr.to_string(), + &target, ]) .output() .unwrap(); @@ -99,8 +100,173 @@ fn single_target_default_table_omits_target_and_accepts_pacing() { String::from_utf8_lossy(&output.stderr) ); let stdout = String::from_utf8(output.stdout).unwrap(); - let header = stdout.lines().next().unwrap_or_default(); - assert!(!header.split_whitespace().any(|column| column == "target")); + let mut lines = stdout.lines(); + let header = lines.next().unwrap_or_default(); + assert_eq!(header.split_whitespace().next(), Some("target")); + assert!( + lines.any(|line| line.split_whitespace().next() == Some(target.as_str())), + "{stdout}" + ); +} + +#[test] +fn single_labeled_target_default_table_includes_target() { + let server = InTreeServer::start(); + + let output = Command::new(env!("CARGO_BIN_EXE_irtt-cli")) + .args([ + "--duration", + "30ms", + "--interval", + "10ms", + "--header", + "always", + "--target", + &format!("eu={}", server.addr), + ]) + .output() + .unwrap(); + + drop(server); + + assert!( + output.status.success(), + "status={:?}\nstderr={}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).unwrap(); + let mut lines = stdout.lines(); + let header = lines.next().unwrap_or_default(); + assert_eq!(header.split_whitespace().next(), Some("target")); + assert!( + lines.any(|line| line.split_whitespace().next() == Some("eu")), + "{stdout}" + ); +} + +#[test] +fn single_labeled_target_default_csv_includes_target_and_event_wall_ns() { + let server = InTreeServer::start(); + + let output = Command::new(env!("CARGO_BIN_EXE_irtt-cli")) + .args([ + "--duration", + "30ms", + "--interval", + "10ms", + "--format", + "csv", + "--header", + "always", + "--target", + &format!("eu={}", server.addr), + ]) + .output() + .unwrap(); + + drop(server); + + assert!( + output.status.success(), + "status={:?}\nstderr={}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).unwrap(); + let mut lines = stdout.lines(); + let header = lines.next().unwrap_or_default(); + assert!(header.starts_with("target,event,"), "{header}"); + assert!( + header.split(',').any(|column| column == "event_wall_ns"), + "{header}" + ); + assert!(lines.any(|line| line.starts_with("eu,")), "{stdout}"); +} + +#[test] +fn single_positional_target_default_csv_includes_target_as_positional_label() { + let server = InTreeServer::start(); + let target = server.addr.to_string(); + + let output = Command::new(env!("CARGO_BIN_EXE_irtt-cli")) + .args([ + "--duration", + "30ms", + "--interval", + "10ms", + "--format", + "csv", + "--header", + "always", + &target, + ]) + .output() + .unwrap(); + + drop(server); + + assert!( + output.status.success(), + "status={:?}\nstderr={}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).unwrap(); + let mut lines = stdout.lines(); + let header = lines.next().unwrap_or_default(); + assert!(header.starts_with("target,event,"), "{header}"); + assert!( + lines.any(|line| line.starts_with(&format!("{target},"))), + "{stdout}" + ); +} + +#[test] +fn multi_target_default_table_includes_both_labels() { + let a = InTreeServer::start(); + let b = InTreeServer::start(); + + let output = Command::new(env!("CARGO_BIN_EXE_irtt-cli")) + .args([ + "--duration", + "40ms", + "--interval", + "10ms", + "--header", + "always", + "--target", + &format!("a={}", a.addr), + "--target", + &format!("b={}", b.addr), + ]) + .output() + .unwrap(); + + drop(a); + drop(b); + + assert!( + output.status.success(), + "status={:?}\nstderr={}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).unwrap(); + let mut lines = stdout.lines(); + let header = lines.next().unwrap_or_default(); + assert_eq!(header.split_whitespace().next(), Some("target")); + let rows: Vec<&str> = lines.collect(); + assert!( + rows.iter() + .any(|line| line.split_whitespace().next() == Some("a")), + "{stdout}" + ); + assert!( + rows.iter() + .any(|line| line.split_whitespace().next() == Some("b")), + "{stdout}" + ); } #[test] @@ -142,6 +308,112 @@ fn single_target_custom_target_column_renders_positional_label() { ); } +#[test] +fn custom_columns_remain_authoritative_and_omit_target() { + let server = InTreeServer::start(); + + let output = Command::new(env!("CARGO_BIN_EXE_irtt-cli")) + .args([ + "--duration", + "30ms", + "--interval", + "10ms", + "--format", + "csv", + "--columns", + "seq,rtt", + "--header", + "always", + &server.addr.to_string(), + ]) + .output() + .unwrap(); + + drop(server); + + assert!( + output.status.success(), + "status={:?}\nstderr={}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).unwrap(); + let header = stdout.lines().next().unwrap_or_default(); + assert_eq!(header, "seq,rtt"); +} + +#[test] +fn columns_default_keyword_includes_target_for_single_target() { + let server = InTreeServer::start(); + + let output = Command::new(env!("CARGO_BIN_EXE_irtt-cli")) + .args([ + "--duration", + "30ms", + "--interval", + "10ms", + "--format", + "csv", + "--columns", + "default", + "--header", + "always", + "--target", + &format!("eu={}", server.addr), + ]) + .output() + .unwrap(); + + drop(server); + + assert!( + output.status.success(), + "status={:?}\nstderr={}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).unwrap(); + let mut lines = stdout.lines(); + let header = lines.next().unwrap_or_default(); + assert!(header.starts_with("target,event,"), "{header}"); + assert!(lines.any(|line| line.starts_with("eu,")), "{stdout}"); +} + +#[test] +fn single_labeled_target_default_jsonl_includes_target() { + let server = InTreeServer::start(); + + let output = Command::new(env!("CARGO_BIN_EXE_irtt-cli")) + .args([ + "--duration", + "30ms", + "--interval", + "10ms", + "--format", + "jsonl", + "--target", + &format!("eu={}", server.addr), + ]) + .output() + .unwrap(); + + drop(server); + + assert!( + output.status.success(), + "status={:?}\nstderr={}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).unwrap(); + assert!( + stdout + .lines() + .any(|line| line.contains("\"target\":\"eu\"")), + "{stdout}" + ); +} + #[test] fn continuous_single_target_peer_close_exits_nonzero() { let server = start_peer_close_server(test_params(None, Duration::from_millis(10)));