Merge pull request #107 from Lochnair/fix/client-target-output

fix(cli): preserve target identity in default output

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<Self, String> {
         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<Vec<Column>, String> {
+fn parse_columns(input: &str, format: OutputFormat, verbose: bool) -> Result<Vec<Column>, 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<Column> {
+fn default_columns(format: OutputFormat, verbose: bool) -> Vec<Column> {
     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\""));
+    }
 }

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();

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)));