From bff90667f3a5402feb6b853b928379d0e3516a45 Mon Sep 17 00:00:00 2001 From: zackees Date: Mon, 22 Jun 2026 04:18:06 -0700 Subject: [PATCH] fix(ci): cargo fmt + split 4 .rs files under 1000 LOC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two pre-existing CI gates were red on main: 1. **Formatting** failed on `crates/fbuild-daemon/src/handlers/websockets.rs`. Resolved by `cargo fmt --all`. The same fmt pass also reflowed the newly-extracted test files. 2. **LOC Gate** flagged 4 files over 1000 lines: crates/fbuild-daemon/src/device_manager.rs (1006, +6) crates/fbuild-cli/src/daemon_client.rs (1049, +49) crates/fbuild-serial/src/manager.rs (1480, +480) crates/fbuild-build/src/source_scanner.rs (1478, +478) For each, lift the `#[cfg(test)] mod tests { ... }` block out of the parent file into a sibling `/tests.rs` submodule and replace it with `#[cfg(test)] mod tests;`. Tests stay in the same module path (`crate::::tests::...`) so test discovery is unchanged; this is purely a textual move that keeps the parent files under the gate. After: device_manager.rs 687 daemon_client.rs 952 manager.rs 972 source_scanner.rs 910 All four are well under 1000 and have room for routine growth before needing further per-domain submodule splits (per the `foo.rs → foo/.rs` convention in CLAUDE.md). Each new `/` directory ships a README per the repo's `readme_guard.py` hook. Verified locally (with SOLDR_NO_ZCCACHE=1 as a workaround for a wedged local zccache daemon — unrelated): - `soldr cargo check --workspace --all-targets` → clean - `soldr cargo fmt --all -- --check` → clean - `soldr cargo clippy --workspace --all-targets -- -D warnings` → clean Co-Authored-By: Claude Opus 4.7 --- crates/fbuild-build/src/source_scanner.rs | 570 +----------------- .../fbuild-build/src/source_scanner/README.md | 13 + .../fbuild-build/src/source_scanner/tests.rs | 567 +++++++++++++++++ crates/fbuild-cli/src/daemon_client.rs | 99 +-- crates/fbuild-cli/src/daemon_client/tests.rs | 99 +++ crates/fbuild-daemon/src/device_manager.rs | 321 +--------- .../src/device_manager/README.md | 10 + .../fbuild-daemon/src/device_manager/tests.rs | 321 ++++++++++ .../fbuild-daemon/src/handlers/websockets.rs | 27 +- crates/fbuild-serial/src/manager.rs | 510 +--------------- crates/fbuild-serial/src/manager/README.md | 12 + crates/fbuild-serial/src/manager/tests.rs | 510 ++++++++++++++++ 12 files changed, 1550 insertions(+), 1509 deletions(-) create mode 100644 crates/fbuild-build/src/source_scanner/README.md create mode 100644 crates/fbuild-build/src/source_scanner/tests.rs create mode 100644 crates/fbuild-cli/src/daemon_client/tests.rs create mode 100644 crates/fbuild-daemon/src/device_manager/README.md create mode 100644 crates/fbuild-daemon/src/device_manager/tests.rs create mode 100644 crates/fbuild-serial/src/manager/README.md create mode 100644 crates/fbuild-serial/src/manager/tests.rs diff --git a/crates/fbuild-build/src/source_scanner.rs b/crates/fbuild-build/src/source_scanner.rs index ea730d70..2fae9307 100644 --- a/crates/fbuild-build/src/source_scanner.rs +++ b/crates/fbuild-build/src/source_scanner.rs @@ -907,572 +907,4 @@ fn trim_trailing_spaces(text: &mut String) { } #[cfg(test)] -mod tests { - use super::*; - use std::fs; - use tempfile::TempDir; - - fn setup_project(src_files: &[(&str, &str)]) -> (TempDir, PathBuf, PathBuf) { - let tmp = TempDir::new().unwrap(); - let src_dir = tmp.path().join("src"); - let build_dir = tmp.path().join("build"); - fs::create_dir_all(&src_dir).unwrap(); - fs::create_dir_all(&build_dir).unwrap(); - - for (name, content) in src_files { - let path = src_dir.join(name); - if let Some(parent) = path.parent() { - fs::create_dir_all(parent).unwrap(); - } - fs::write(&path, content).unwrap(); - } - - (tmp, src_dir, build_dir) - } - - #[test] - fn test_scan_empty_directory() { - let tmp = TempDir::new().unwrap(); - let src_dir = tmp.path().join("src"); - let build_dir = tmp.path().join("build"); - fs::create_dir_all(&src_dir).unwrap(); - fs::create_dir_all(&build_dir).unwrap(); - - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - assert!(sources.is_empty()); - } - - #[test] - fn test_nonexistent_source_directory() { - let tmp = TempDir::new().unwrap(); - let scanner = - SourceScanner::new(&tmp.path().join("nonexistent"), &tmp.path().join("build")); - let sources = scanner.scan_sketch_sources().unwrap(); - assert!(sources.is_empty()); - } - - #[test] - fn test_scan_cpp_files() { - let (_tmp, src_dir, build_dir) = setup_project(&[("main.cpp", "int main() { return 0; }")]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - assert_eq!(sources.len(), 1); - assert!(sources[0].to_string_lossy().contains("main.cpp")); - } - - #[test] - fn test_scan_cxx_files() { - let (_tmp, src_dir, build_dir) = setup_project(&[("helper.cxx", "void helper() {}")]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - assert_eq!(sources.len(), 1); - assert!(sources[0].to_string_lossy().contains("helper.cxx")); - } - - #[test] - fn test_scan_c_files() { - let (_tmp, src_dir, build_dir) = setup_project(&[("helper.c", "void helper() {}")]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - assert_eq!(sources.len(), 1); - assert!(sources[0].to_string_lossy().contains("helper.c")); - } - - #[test] - fn test_scan_single_ino_file() { - let (_tmp, src_dir, build_dir) = - setup_project(&[("sketch.ino", "void setup() {}\nvoid loop() {}\n")]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - assert_eq!(sources.len(), 1); - assert!(sources[0].to_string_lossy().contains(".ino.cpp")); - - // Direct sketch scans do not know framework include roots. - let content = fs::read_to_string(&sources[0]).unwrap(); - assert!(!content.contains("#include ")); - } - - #[test] - fn test_scan_multiple_ino_files() { - let (_tmp, src_dir, build_dir) = setup_project(&[ - ("a.ino", "void helperA() {}\n"), - ("b.ino", "void helperB() {}\n"), - ]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - assert_eq!(sources.len(), 1); // Concatenated into one .cpp - let content = fs::read_to_string(&sources[0]).unwrap(); - assert!(content.contains("helperA")); - assert!(content.contains("helperB")); - } - - #[test] - fn test_scan_multiple_ino_files_uses_platformio_main_first() { - let (_tmp, src_dir, build_dir) = setup_project(&[ - ("z_tab.ino", "void zTab() {}\n"), - ("main.ino", "void setup() {}\nvoid loop() {}\n"), - ("a_tab.ino", "void aTab() {}\n"), - ]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - - let sources = scanner.scan_sketch_sources().unwrap(); - assert_eq!(sources.len(), 1); - assert!(sources[0].ends_with("main.ino.cpp")); - let content = fs::read_to_string(&sources[0]).unwrap(); - - let main_pos = content.find("void setup()").unwrap(); - let a_pos = content.find("void aTab()").unwrap(); - let z_pos = content.find("void zTab()").unwrap(); - assert!(main_pos < a_pos); - assert!(a_pos < z_pos); - } - - #[test] - fn test_scan_multiple_ino_files_uses_arduino_named_primary_first() { - let tmp = TempDir::new().unwrap(); - let src_dir = tmp.path().join("Blink"); - let build_dir = tmp.path().join("build"); - fs::create_dir_all(&src_dir).unwrap(); - fs::create_dir_all(&build_dir).unwrap(); - fs::write(src_dir.join("z_tab.ino"), "void zTab() {}\n").unwrap(); - fs::write( - src_dir.join("Blink.ino"), - "void setup() {}\nvoid loop() {}\n", - ) - .unwrap(); - fs::write(src_dir.join("a_tab.ino"), "void aTab() {}\n").unwrap(); - let scanner = SourceScanner::new(&src_dir, &build_dir); - - let sources = scanner.scan_sketch_sources().unwrap(); - assert_eq!(sources.len(), 1); - assert!(sources[0].ends_with("Blink.ino.cpp")); - let content = fs::read_to_string(&sources[0]).unwrap(); - - let primary_pos = content.find("void setup()").unwrap(); - let a_pos = content.find("void aTab()").unwrap(); - let z_pos = content.find("void zTab()").unwrap(); - assert!(primary_pos < a_pos); - assert!(a_pos < z_pos); - } - - #[test] - fn test_scan_multiple_ino_files_falls_back_to_setup_loop_primary() { - let (_tmp, src_dir, build_dir) = setup_project(&[ - ("a_tab.ino", "void aTab() {}\n"), - ("z_entry.ino", "void setup() {}\nvoid loop() {}\n"), - ("b_tab.ino", "void bTab() {}\n"), - ]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - - let sources = scanner.scan_sketch_sources().unwrap(); - assert_eq!(sources.len(), 1); - assert!(sources[0].ends_with("z_entry.ino.cpp")); - let content = fs::read_to_string(&sources[0]).unwrap(); - - let primary_pos = content.find("void setup()").unwrap(); - let a_pos = content.find("void aTab()").unwrap(); - let b_pos = content.find("void bTab()").unwrap(); - assert!(primary_pos < a_pos); - assert!(a_pos < b_pos); - } - - #[test] - fn test_scan_mixed_sources() { - let (_tmp, src_dir, build_dir) = setup_project(&[ - ("sketch.ino", "void setup() {}\nvoid loop() {}\n"), - ("helper.cpp", "void helper() {}"), - ("util.c", "void util() {}"), - ]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - assert_eq!(sources.len(), 3); // 1 preprocessed ino + 2 others - } - - #[test] - fn test_scan_main_cpp_with_ino_skips_preprocessing_but_keeps_main_cpp() { - let (_tmp, src_dir, build_dir) = setup_project(&[ - ("main.cpp", "#include \"sketch.ino\"\n"), - ("sketch.ino", "void setup() {}\nvoid loop() {}\n"), - ]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - - let sources = scanner.scan_sketch_sources().unwrap(); - - assert_eq!(sources.len(), 1); - assert!(sources[0].ends_with("main.cpp")); - assert!(!build_dir.join("sketch.ino.cpp").exists()); - } - - #[test] - fn test_main_cpp_with_ino_warning_is_yellow_and_clear() { - let tmp = TempDir::new().unwrap(); - let main_cpp = tmp.path().join("src").join("main.cpp"); - let ino = tmp.path().join("src").join("sketch.ino"); - let mut out = Vec::new(); - - write_main_cpp_skips_ino_warning(&mut out, &main_cpp, &[ino]).unwrap(); - let warning = String::from_utf8(out).unwrap(); - - assert!(warning.contains("\u{1b}[")); - assert!(warning.contains("warning:")); - assert!(warning.contains("main.cpp takes precedence")); - assert!(warning.contains("skipping automatic .ino preprocessing")); - assert!(warning.contains("sketch.ino")); - } - - #[test] - fn test_main_cpp_without_ino_warning_is_silent() { - let tmp = TempDir::new().unwrap(); - let main_cpp = tmp.path().join("src").join("main.cpp"); - let mut out = Vec::new(); - - write_main_cpp_skips_ino_warning(&mut out, &main_cpp, &[]).unwrap(); - - assert!(out.is_empty()); - } - - #[test] - fn test_scan_headers() { - let (_tmp, src_dir, build_dir) = setup_project(&[ - ("main.cpp", ""), - ("header.h", "#pragma once"), - ("header2.hpp", "#pragma once"), - ]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let headers = scanner.scan_headers(&src_dir); - assert_eq!(headers.len(), 2); - } - - #[test] - fn test_scan_subdirectories() { - let (_tmp, src_dir, build_dir) = setup_project(&[ - ("main.cpp", ""), - ("sub/helper.cpp", ""), - ("sub/deep/util.c", ""), - ]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - assert_eq!(sources.len(), 3); - } - - #[test] - fn test_scan_core_sources() { - let tmp = TempDir::new().unwrap(); - let core_dir = tmp.path().join("cores/arduino"); - fs::create_dir_all(&core_dir).unwrap(); - fs::write(core_dir.join("main.cpp"), "int main() {}").unwrap(); - fs::write(core_dir.join("wiring.c"), "void init() {}").unwrap(); - fs::write(core_dir.join("helper.cxx"), "void helper() {}").unwrap(); - fs::write(core_dir.join("Arduino.h"), "#pragma once").unwrap(); - - let scanner = SourceScanner::new(&tmp.path().join("src"), &tmp.path().join("build")); - let sources = scanner.scan_core_sources(&core_dir); - assert_eq!(sources.len(), 3); // .cpp, .c, and .cxx, not .h - assert!(sources.iter().any(|p| p.ends_with("helper.cxx"))); - } - - #[test] - fn test_nonexistent_core_directory() { - let tmp = TempDir::new().unwrap(); - let scanner = SourceScanner::new(&tmp.path().join("src"), &tmp.path().join("build")); - let sources = scanner.scan_core_sources(&tmp.path().join("nonexistent")); - assert!(sources.is_empty()); - } - - #[test] - fn test_scan_variant_cxx_sources() { - let tmp = TempDir::new().unwrap(); - let variant_dir = tmp.path().join("variants/demo"); - fs::create_dir_all(&variant_dir).unwrap(); - fs::write(variant_dir.join("variant.cxx"), "void variant() {}").unwrap(); - fs::write(variant_dir.join("variant.h"), "#pragma once").unwrap(); - - let scanner = SourceScanner::new(&tmp.path().join("src"), &tmp.path().join("build")); - let sources = scanner.scan_variant_sources(&variant_dir); - - assert_eq!(sources, vec![variant_dir.join("variant.cxx")]); - } - - #[test] - fn test_preprocess_simple_ino() { - let (_tmp, src_dir, build_dir) = setup_project(&[( - "sketch.ino", - "void setup() {\n pinMode(13, OUTPUT);\n}\n\nvoid loop() {\n digitalWrite(13, HIGH);\n}\n", - )]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - let content = fs::read_to_string(&sources[0]).unwrap(); - - assert!(!content.contains("#include ")); - assert!(content.contains("void setup()")); - assert!(content.contains("void loop()")); - } - - #[test] - fn test_preprocess_includes_arduino_h_when_header_available() { - let tmp = TempDir::new().unwrap(); - let src_dir = tmp.path().join("src"); - let build_dir = tmp.path().join("build"); - let core_dir = tmp.path().join("core"); - fs::create_dir_all(&src_dir).unwrap(); - fs::create_dir_all(&core_dir).unwrap(); - fs::write( - src_dir.join("sketch.ino"), - "void setup() {}\nvoid loop() {}\n", - ) - .unwrap(); - fs::write(core_dir.join("Arduino.h"), "#pragma once\n").unwrap(); - - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner - .scan_all(Some(&core_dir), None) - .unwrap() - .sketch_sources; - let content = fs::read_to_string(&sources[0]).unwrap(); - - assert!(content.contains("#include ")); - } - - #[test] - fn test_preprocess_with_custom_functions() { - let (_tmp, src_dir, build_dir) = setup_project(&[( - "sketch.ino", - "int add(int a, int b) {\n return a + b;\n}\n\nvoid setup() {\n int x = add(1, 2);\n}\n\nvoid loop() {}\n", - )]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - let content = fs::read_to_string(&sources[0]).unwrap(); - - // Should have auto-generated prototypes - assert!(content.contains("int add(int a, int b)")); - assert!(content.contains("void setup()")); - } - - #[test] - fn test_function_prototype_extraction() { - let source = "void setup() {\n}\nint compute(float x, int y) {\n return 0;\n}\nconst char* getName() {\n return \"\";\n}\n"; - let protos = extract_function_prototypes(source); - assert!(protos.len() >= 2); - assert!(protos.iter().any(|p| p.contains("setup"))); - assert!(protos.iter().any(|p| p.contains("compute"))); - } - - #[test] - fn test_prototype_extraction_handles_complex_cpp_signatures() { - let source = r#" -template -T twice(T value) { - return value + value; -} - -[[nodiscard]] const char* label(const char* fallback = "demo") { - return fallback; -} - -int& ref_value(int& value) { - return value; -} -"#; - let protos = extract_function_prototypes(source); - assert!(protos.contains(&"template T twice(T value)".to_string())); - assert!( - protos.contains(&"[[nodiscard]] const char* label(const char* fallback)".to_string()) - ); - assert!(protos.contains(&"int& ref_value(int& value)".to_string())); - assert!(!protos.iter().any(|p| p.contains("= \"demo\""))); - } - - #[test] - fn test_prototype_extraction_skips_non_free_functions() { - let source = r#" -#define MAKE_FUNC(name) void name() {} - -void setup() { - if (true) { - } - while (false) { - } - auto callback = []() { return 1; }; -} - -class Controller { - void tick() {} -}; - -namespace hidden { -void helper() {} -} - -void Controller::external_tick() {} -"#; - let protos = extract_function_prototypes(source); - assert!(protos.iter().any(|p| p == "void setup()")); - assert!(!protos.iter().any(|p| p.contains("if"))); - assert!(!protos.iter().any(|p| p.contains("while"))); - assert!(!protos.iter().any(|p| p.contains("callback"))); - assert!(!protos.iter().any(|p| p.contains("tick"))); - assert!(!protos.iter().any(|p| p.contains("helper"))); - assert!(!protos.iter().any(|p| p.contains("MAKE_FUNC"))); - } - - #[test] - fn test_line_numbers_preserved() { - let (_tmp, src_dir, build_dir) = - setup_project(&[("sketch.ino", "void setup() {}\nvoid loop() {}\n")]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - let content = fs::read_to_string(&sources[0]).unwrap(); - assert!(content.contains("#line 1")); - } - - #[test] - fn test_line_directive_path_is_project_relative_and_slash_normalized() { - let (_tmp, src_dir, build_dir) = - setup_project(&[("sketch.ino", "void setup() {}\nvoid loop() {}\n")]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - let content = fs::read_to_string(&sources[0]).unwrap(); - - assert!(content.contains("#line 1 \"src/sketch.ino\"")); - assert!(!content.contains('\\')); - } - - #[test] - fn test_generated_ino_cpp_uses_lf_line_endings() { - let (_tmp, src_dir, build_dir) = - setup_project(&[("sketch.ino", "void setup() {}\r\nvoid loop() {}\r\n")]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner.scan_sketch_sources().unwrap(); - let content = fs::read_to_string(&sources[0]).unwrap(); - - assert!(!content.contains("\r\n")); - } - - #[test] - fn test_windows_style_generated_path_text_is_stable() { - assert_eq!( - normalize_generated_source_path_text(r"C:\Users\dev\project\src\main.ino"), - "c:/Users/dev/project/src/main.ino" - ); - assert_eq!( - normalize_generated_source_path_text(r"C:\Users\dev\project/src\main.ino"), - "c:/Users/dev/project/src/main.ino" - ); - assert_eq!( - normalize_generated_source_path_text("src\\main.ino"), - "src/main.ino" - ); - } - - #[test] - fn test_preprocess_does_not_rewrite_unchanged_output() { - let (_tmp, src_dir, build_dir) = - setup_project(&[("sketch.ino", "void setup() {}\nvoid loop() {}\n")]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - - let first = scanner.scan_sketch_sources().unwrap(); - let output = first[0].clone(); - let first_mtime = fs::metadata(&output).unwrap().modified().unwrap(); - - std::thread::sleep(std::time::Duration::from_millis(20)); - - let second = scanner.scan_sketch_sources().unwrap(); - assert_eq!(second[0], output); - let second_mtime = fs::metadata(&output).unwrap().modified().unwrap(); - - assert_eq!(first_mtime, second_mtime); - } - - #[test] - fn test_preprocess_with_arduino_h_does_not_rewrite_unchanged_output() { - let tmp = TempDir::new().unwrap(); - let src_dir = tmp.path().join("src"); - let build_dir = tmp.path().join("build"); - let core_dir = tmp.path().join("core"); - fs::create_dir_all(&src_dir).unwrap(); - fs::create_dir_all(&core_dir).unwrap(); - fs::write( - src_dir.join("sketch.ino"), - "void setup() {}\nvoid loop() {}\n", - ) - .unwrap(); - fs::write(core_dir.join("Arduino.h"), "#pragma once\n").unwrap(); - let scanner = SourceScanner::new(&src_dir, &build_dir); - - let first = scanner - .scan_all(Some(&core_dir), None) - .unwrap() - .sketch_sources; - let output = first[0].clone(); - let first_mtime = fs::metadata(&output).unwrap().modified().unwrap(); - - std::thread::sleep(std::time::Duration::from_millis(20)); - - let second = scanner - .scan_all(Some(&core_dir), None) - .unwrap() - .sketch_sources; - assert_eq!(second[0], output); - let second_mtime = fs::metadata(&output).unwrap().modified().unwrap(); - - assert_eq!(first_mtime, second_mtime); - } - - #[test] - fn test_source_collection_all_sources() { - let tmp = TempDir::new().unwrap(); - let src_dir = tmp.path().join("src"); - let core_dir = tmp.path().join("core"); - let variant_dir = tmp.path().join("variant"); - let build_dir = tmp.path().join("build"); - fs::create_dir_all(&src_dir).unwrap(); - fs::create_dir_all(&core_dir).unwrap(); - fs::create_dir_all(&variant_dir).unwrap(); - fs::write(src_dir.join("main.cpp"), "").unwrap(); - fs::write(core_dir.join("core.cpp"), "").unwrap(); - fs::write(variant_dir.join("variant.c"), "").unwrap(); - - let scanner = SourceScanner::new(&src_dir, &build_dir); - let collection = scanner - .scan_all(Some(&core_dir), Some(&variant_dir)) - .unwrap(); - assert_eq!(collection.sketch_sources.len(), 1); - assert_eq!(collection.core_sources.len(), 1); - assert_eq!(collection.variant_sources.len(), 1); - assert_eq!(collection.all_sources().len(), 3); - } - - #[test] - fn test_scan_sketch_sources_filtered_excludes_subdirectory() { - let (_tmp, src_dir, build_dir) = setup_project(&[ - ("main.cpp", "int main() { return 0; }"), - ("generated/skip.cpp", "void skip() {}"), - ]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner - .scan_sketch_sources_filtered(Some("+<*>\n-")) - .unwrap(); - assert_eq!(sources.len(), 1); - assert!(sources[0].ends_with("main.cpp")); - } - - #[test] - fn test_scan_sketch_sources_filtered_includes_only_selected_files() { - let (_tmp, src_dir, build_dir) = setup_project(&[ - ("main.cpp", "int main() { return 0; }"), - ("helper.cpp", "void helper() {}"), - ("sub/util.c", "void util() {}"), - ]); - let scanner = SourceScanner::new(&src_dir, &build_dir); - let sources = scanner - .scan_sketch_sources_filtered(Some("+\n+")) - .unwrap(); - assert_eq!(sources.len(), 2); - assert!(sources.iter().any(|p| p.ends_with("main.cpp"))); - assert!(sources - .iter() - .any(|p| p.ends_with("sub\\util.c") || p.ends_with("sub/util.c"))); - assert!(!sources.iter().any(|p| p.ends_with("helper.cpp"))); - } -} +mod tests; diff --git a/crates/fbuild-build/src/source_scanner/README.md b/crates/fbuild-build/src/source_scanner/README.md new file mode 100644 index 00000000..934d395e --- /dev/null +++ b/crates/fbuild-build/src/source_scanner/README.md @@ -0,0 +1,13 @@ +# `source_scanner/` + +Sibling-directory submodules for `source_scanner.rs` (the project source +discovery + filtering + prototype-extraction code). Today this is just +`tests.rs` — the original `#[cfg(test)] mod tests { ... }` block was +lifted out of the parent file as a separate file so the parent stays +under the 1000-LOC gate (see `.github/workflows/loc-gate.yml`). + +When growing this module, prefer cohesive per-domain submodules +(`filter.rs` for the include/exclude rule engine, `prototypes.rs` for +the tree-sitter-based forward-declaration extraction, …) over letting +the parent file balloon back over the limit. The split convention is +documented in the root `CLAUDE.md`. diff --git a/crates/fbuild-build/src/source_scanner/tests.rs b/crates/fbuild-build/src/source_scanner/tests.rs new file mode 100644 index 00000000..d75ed11a --- /dev/null +++ b/crates/fbuild-build/src/source_scanner/tests.rs @@ -0,0 +1,567 @@ +//! Unit tests for the parent `source_scanner` module. Extracted to keep the +//! parent file under the 1000-LOC gate (see ci.yml LOC Gate workflow). + +use super::*; +use std::fs; +use tempfile::TempDir; + +fn setup_project(src_files: &[(&str, &str)]) -> (TempDir, PathBuf, PathBuf) { + let tmp = TempDir::new().unwrap(); + let src_dir = tmp.path().join("src"); + let build_dir = tmp.path().join("build"); + fs::create_dir_all(&src_dir).unwrap(); + fs::create_dir_all(&build_dir).unwrap(); + + for (name, content) in src_files { + let path = src_dir.join(name); + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).unwrap(); + } + fs::write(&path, content).unwrap(); + } + + (tmp, src_dir, build_dir) +} + +#[test] +fn test_scan_empty_directory() { + let tmp = TempDir::new().unwrap(); + let src_dir = tmp.path().join("src"); + let build_dir = tmp.path().join("build"); + fs::create_dir_all(&src_dir).unwrap(); + fs::create_dir_all(&build_dir).unwrap(); + + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + assert!(sources.is_empty()); +} + +#[test] +fn test_nonexistent_source_directory() { + let tmp = TempDir::new().unwrap(); + let scanner = SourceScanner::new(&tmp.path().join("nonexistent"), &tmp.path().join("build")); + let sources = scanner.scan_sketch_sources().unwrap(); + assert!(sources.is_empty()); +} + +#[test] +fn test_scan_cpp_files() { + let (_tmp, src_dir, build_dir) = setup_project(&[("main.cpp", "int main() { return 0; }")]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + assert_eq!(sources.len(), 1); + assert!(sources[0].to_string_lossy().contains("main.cpp")); +} + +#[test] +fn test_scan_cxx_files() { + let (_tmp, src_dir, build_dir) = setup_project(&[("helper.cxx", "void helper() {}")]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + assert_eq!(sources.len(), 1); + assert!(sources[0].to_string_lossy().contains("helper.cxx")); +} + +#[test] +fn test_scan_c_files() { + let (_tmp, src_dir, build_dir) = setup_project(&[("helper.c", "void helper() {}")]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + assert_eq!(sources.len(), 1); + assert!(sources[0].to_string_lossy().contains("helper.c")); +} + +#[test] +fn test_scan_single_ino_file() { + let (_tmp, src_dir, build_dir) = + setup_project(&[("sketch.ino", "void setup() {}\nvoid loop() {}\n")]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + assert_eq!(sources.len(), 1); + assert!(sources[0].to_string_lossy().contains(".ino.cpp")); + + // Direct sketch scans do not know framework include roots. + let content = fs::read_to_string(&sources[0]).unwrap(); + assert!(!content.contains("#include ")); +} + +#[test] +fn test_scan_multiple_ino_files() { + let (_tmp, src_dir, build_dir) = setup_project(&[ + ("a.ino", "void helperA() {}\n"), + ("b.ino", "void helperB() {}\n"), + ]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + assert_eq!(sources.len(), 1); // Concatenated into one .cpp + let content = fs::read_to_string(&sources[0]).unwrap(); + assert!(content.contains("helperA")); + assert!(content.contains("helperB")); +} + +#[test] +fn test_scan_multiple_ino_files_uses_platformio_main_first() { + let (_tmp, src_dir, build_dir) = setup_project(&[ + ("z_tab.ino", "void zTab() {}\n"), + ("main.ino", "void setup() {}\nvoid loop() {}\n"), + ("a_tab.ino", "void aTab() {}\n"), + ]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + + let sources = scanner.scan_sketch_sources().unwrap(); + assert_eq!(sources.len(), 1); + assert!(sources[0].ends_with("main.ino.cpp")); + let content = fs::read_to_string(&sources[0]).unwrap(); + + let main_pos = content.find("void setup()").unwrap(); + let a_pos = content.find("void aTab()").unwrap(); + let z_pos = content.find("void zTab()").unwrap(); + assert!(main_pos < a_pos); + assert!(a_pos < z_pos); +} + +#[test] +fn test_scan_multiple_ino_files_uses_arduino_named_primary_first() { + let tmp = TempDir::new().unwrap(); + let src_dir = tmp.path().join("Blink"); + let build_dir = tmp.path().join("build"); + fs::create_dir_all(&src_dir).unwrap(); + fs::create_dir_all(&build_dir).unwrap(); + fs::write(src_dir.join("z_tab.ino"), "void zTab() {}\n").unwrap(); + fs::write( + src_dir.join("Blink.ino"), + "void setup() {}\nvoid loop() {}\n", + ) + .unwrap(); + fs::write(src_dir.join("a_tab.ino"), "void aTab() {}\n").unwrap(); + let scanner = SourceScanner::new(&src_dir, &build_dir); + + let sources = scanner.scan_sketch_sources().unwrap(); + assert_eq!(sources.len(), 1); + assert!(sources[0].ends_with("Blink.ino.cpp")); + let content = fs::read_to_string(&sources[0]).unwrap(); + + let primary_pos = content.find("void setup()").unwrap(); + let a_pos = content.find("void aTab()").unwrap(); + let z_pos = content.find("void zTab()").unwrap(); + assert!(primary_pos < a_pos); + assert!(a_pos < z_pos); +} + +#[test] +fn test_scan_multiple_ino_files_falls_back_to_setup_loop_primary() { + let (_tmp, src_dir, build_dir) = setup_project(&[ + ("a_tab.ino", "void aTab() {}\n"), + ("z_entry.ino", "void setup() {}\nvoid loop() {}\n"), + ("b_tab.ino", "void bTab() {}\n"), + ]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + + let sources = scanner.scan_sketch_sources().unwrap(); + assert_eq!(sources.len(), 1); + assert!(sources[0].ends_with("z_entry.ino.cpp")); + let content = fs::read_to_string(&sources[0]).unwrap(); + + let primary_pos = content.find("void setup()").unwrap(); + let a_pos = content.find("void aTab()").unwrap(); + let b_pos = content.find("void bTab()").unwrap(); + assert!(primary_pos < a_pos); + assert!(a_pos < b_pos); +} + +#[test] +fn test_scan_mixed_sources() { + let (_tmp, src_dir, build_dir) = setup_project(&[ + ("sketch.ino", "void setup() {}\nvoid loop() {}\n"), + ("helper.cpp", "void helper() {}"), + ("util.c", "void util() {}"), + ]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + assert_eq!(sources.len(), 3); // 1 preprocessed ino + 2 others +} + +#[test] +fn test_scan_main_cpp_with_ino_skips_preprocessing_but_keeps_main_cpp() { + let (_tmp, src_dir, build_dir) = setup_project(&[ + ("main.cpp", "#include \"sketch.ino\"\n"), + ("sketch.ino", "void setup() {}\nvoid loop() {}\n"), + ]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + + let sources = scanner.scan_sketch_sources().unwrap(); + + assert_eq!(sources.len(), 1); + assert!(sources[0].ends_with("main.cpp")); + assert!(!build_dir.join("sketch.ino.cpp").exists()); +} + +#[test] +fn test_main_cpp_with_ino_warning_is_yellow_and_clear() { + let tmp = TempDir::new().unwrap(); + let main_cpp = tmp.path().join("src").join("main.cpp"); + let ino = tmp.path().join("src").join("sketch.ino"); + let mut out = Vec::new(); + + write_main_cpp_skips_ino_warning(&mut out, &main_cpp, &[ino]).unwrap(); + let warning = String::from_utf8(out).unwrap(); + + assert!(warning.contains("\u{1b}[")); + assert!(warning.contains("warning:")); + assert!(warning.contains("main.cpp takes precedence")); + assert!(warning.contains("skipping automatic .ino preprocessing")); + assert!(warning.contains("sketch.ino")); +} + +#[test] +fn test_main_cpp_without_ino_warning_is_silent() { + let tmp = TempDir::new().unwrap(); + let main_cpp = tmp.path().join("src").join("main.cpp"); + let mut out = Vec::new(); + + write_main_cpp_skips_ino_warning(&mut out, &main_cpp, &[]).unwrap(); + + assert!(out.is_empty()); +} + +#[test] +fn test_scan_headers() { + let (_tmp, src_dir, build_dir) = setup_project(&[ + ("main.cpp", ""), + ("header.h", "#pragma once"), + ("header2.hpp", "#pragma once"), + ]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let headers = scanner.scan_headers(&src_dir); + assert_eq!(headers.len(), 2); +} + +#[test] +fn test_scan_subdirectories() { + let (_tmp, src_dir, build_dir) = setup_project(&[ + ("main.cpp", ""), + ("sub/helper.cpp", ""), + ("sub/deep/util.c", ""), + ]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + assert_eq!(sources.len(), 3); +} + +#[test] +fn test_scan_core_sources() { + let tmp = TempDir::new().unwrap(); + let core_dir = tmp.path().join("cores/arduino"); + fs::create_dir_all(&core_dir).unwrap(); + fs::write(core_dir.join("main.cpp"), "int main() {}").unwrap(); + fs::write(core_dir.join("wiring.c"), "void init() {}").unwrap(); + fs::write(core_dir.join("helper.cxx"), "void helper() {}").unwrap(); + fs::write(core_dir.join("Arduino.h"), "#pragma once").unwrap(); + + let scanner = SourceScanner::new(&tmp.path().join("src"), &tmp.path().join("build")); + let sources = scanner.scan_core_sources(&core_dir); + assert_eq!(sources.len(), 3); // .cpp, .c, and .cxx, not .h + assert!(sources.iter().any(|p| p.ends_with("helper.cxx"))); +} + +#[test] +fn test_nonexistent_core_directory() { + let tmp = TempDir::new().unwrap(); + let scanner = SourceScanner::new(&tmp.path().join("src"), &tmp.path().join("build")); + let sources = scanner.scan_core_sources(&tmp.path().join("nonexistent")); + assert!(sources.is_empty()); +} + +#[test] +fn test_scan_variant_cxx_sources() { + let tmp = TempDir::new().unwrap(); + let variant_dir = tmp.path().join("variants/demo"); + fs::create_dir_all(&variant_dir).unwrap(); + fs::write(variant_dir.join("variant.cxx"), "void variant() {}").unwrap(); + fs::write(variant_dir.join("variant.h"), "#pragma once").unwrap(); + + let scanner = SourceScanner::new(&tmp.path().join("src"), &tmp.path().join("build")); + let sources = scanner.scan_variant_sources(&variant_dir); + + assert_eq!(sources, vec![variant_dir.join("variant.cxx")]); +} + +#[test] +fn test_preprocess_simple_ino() { + let (_tmp, src_dir, build_dir) = setup_project(&[( + "sketch.ino", + "void setup() {\n pinMode(13, OUTPUT);\n}\n\nvoid loop() {\n digitalWrite(13, HIGH);\n}\n", + )]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + let content = fs::read_to_string(&sources[0]).unwrap(); + + assert!(!content.contains("#include ")); + assert!(content.contains("void setup()")); + assert!(content.contains("void loop()")); +} + +#[test] +fn test_preprocess_includes_arduino_h_when_header_available() { + let tmp = TempDir::new().unwrap(); + let src_dir = tmp.path().join("src"); + let build_dir = tmp.path().join("build"); + let core_dir = tmp.path().join("core"); + fs::create_dir_all(&src_dir).unwrap(); + fs::create_dir_all(&core_dir).unwrap(); + fs::write( + src_dir.join("sketch.ino"), + "void setup() {}\nvoid loop() {}\n", + ) + .unwrap(); + fs::write(core_dir.join("Arduino.h"), "#pragma once\n").unwrap(); + + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner + .scan_all(Some(&core_dir), None) + .unwrap() + .sketch_sources; + let content = fs::read_to_string(&sources[0]).unwrap(); + + assert!(content.contains("#include ")); +} + +#[test] +fn test_preprocess_with_custom_functions() { + let (_tmp, src_dir, build_dir) = setup_project(&[( + "sketch.ino", + "int add(int a, int b) {\n return a + b;\n}\n\nvoid setup() {\n int x = add(1, 2);\n}\n\nvoid loop() {}\n", + )]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + let content = fs::read_to_string(&sources[0]).unwrap(); + + // Should have auto-generated prototypes + assert!(content.contains("int add(int a, int b)")); + assert!(content.contains("void setup()")); +} + +#[test] +fn test_function_prototype_extraction() { + let source = "void setup() {\n}\nint compute(float x, int y) {\n return 0;\n}\nconst char* getName() {\n return \"\";\n}\n"; + let protos = extract_function_prototypes(source); + assert!(protos.len() >= 2); + assert!(protos.iter().any(|p| p.contains("setup"))); + assert!(protos.iter().any(|p| p.contains("compute"))); +} + +#[test] +fn test_prototype_extraction_handles_complex_cpp_signatures() { + let source = r#" +template +T twice(T value) { + return value + value; +} + +[[nodiscard]] const char* label(const char* fallback = "demo") { + return fallback; +} + +int& ref_value(int& value) { + return value; +} +"#; + let protos = extract_function_prototypes(source); + assert!(protos.contains(&"template T twice(T value)".to_string())); + assert!(protos.contains(&"[[nodiscard]] const char* label(const char* fallback)".to_string())); + assert!(protos.contains(&"int& ref_value(int& value)".to_string())); + assert!(!protos.iter().any(|p| p.contains("= \"demo\""))); +} + +#[test] +fn test_prototype_extraction_skips_non_free_functions() { + let source = r#" +#define MAKE_FUNC(name) void name() {} + +void setup() { + if (true) { + } + while (false) { + } + auto callback = []() { return 1; }; +} + +class Controller { + void tick() {} +}; + +namespace hidden { +void helper() {} +} + +void Controller::external_tick() {} +"#; + let protos = extract_function_prototypes(source); + assert!(protos.iter().any(|p| p == "void setup()")); + assert!(!protos.iter().any(|p| p.contains("if"))); + assert!(!protos.iter().any(|p| p.contains("while"))); + assert!(!protos.iter().any(|p| p.contains("callback"))); + assert!(!protos.iter().any(|p| p.contains("tick"))); + assert!(!protos.iter().any(|p| p.contains("helper"))); + assert!(!protos.iter().any(|p| p.contains("MAKE_FUNC"))); +} + +#[test] +fn test_line_numbers_preserved() { + let (_tmp, src_dir, build_dir) = + setup_project(&[("sketch.ino", "void setup() {}\nvoid loop() {}\n")]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + let content = fs::read_to_string(&sources[0]).unwrap(); + assert!(content.contains("#line 1")); +} + +#[test] +fn test_line_directive_path_is_project_relative_and_slash_normalized() { + let (_tmp, src_dir, build_dir) = + setup_project(&[("sketch.ino", "void setup() {}\nvoid loop() {}\n")]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + let content = fs::read_to_string(&sources[0]).unwrap(); + + assert!(content.contains("#line 1 \"src/sketch.ino\"")); + assert!(!content.contains('\\')); +} + +#[test] +fn test_generated_ino_cpp_uses_lf_line_endings() { + let (_tmp, src_dir, build_dir) = + setup_project(&[("sketch.ino", "void setup() {}\r\nvoid loop() {}\r\n")]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner.scan_sketch_sources().unwrap(); + let content = fs::read_to_string(&sources[0]).unwrap(); + + assert!(!content.contains("\r\n")); +} + +#[test] +fn test_windows_style_generated_path_text_is_stable() { + assert_eq!( + normalize_generated_source_path_text(r"C:\Users\dev\project\src\main.ino"), + "c:/Users/dev/project/src/main.ino" + ); + assert_eq!( + normalize_generated_source_path_text(r"C:\Users\dev\project/src\main.ino"), + "c:/Users/dev/project/src/main.ino" + ); + assert_eq!( + normalize_generated_source_path_text("src\\main.ino"), + "src/main.ino" + ); +} + +#[test] +fn test_preprocess_does_not_rewrite_unchanged_output() { + let (_tmp, src_dir, build_dir) = + setup_project(&[("sketch.ino", "void setup() {}\nvoid loop() {}\n")]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + + let first = scanner.scan_sketch_sources().unwrap(); + let output = first[0].clone(); + let first_mtime = fs::metadata(&output).unwrap().modified().unwrap(); + + std::thread::sleep(std::time::Duration::from_millis(20)); + + let second = scanner.scan_sketch_sources().unwrap(); + assert_eq!(second[0], output); + let second_mtime = fs::metadata(&output).unwrap().modified().unwrap(); + + assert_eq!(first_mtime, second_mtime); +} + +#[test] +fn test_preprocess_with_arduino_h_does_not_rewrite_unchanged_output() { + let tmp = TempDir::new().unwrap(); + let src_dir = tmp.path().join("src"); + let build_dir = tmp.path().join("build"); + let core_dir = tmp.path().join("core"); + fs::create_dir_all(&src_dir).unwrap(); + fs::create_dir_all(&core_dir).unwrap(); + fs::write( + src_dir.join("sketch.ino"), + "void setup() {}\nvoid loop() {}\n", + ) + .unwrap(); + fs::write(core_dir.join("Arduino.h"), "#pragma once\n").unwrap(); + let scanner = SourceScanner::new(&src_dir, &build_dir); + + let first = scanner + .scan_all(Some(&core_dir), None) + .unwrap() + .sketch_sources; + let output = first[0].clone(); + let first_mtime = fs::metadata(&output).unwrap().modified().unwrap(); + + std::thread::sleep(std::time::Duration::from_millis(20)); + + let second = scanner + .scan_all(Some(&core_dir), None) + .unwrap() + .sketch_sources; + assert_eq!(second[0], output); + let second_mtime = fs::metadata(&output).unwrap().modified().unwrap(); + + assert_eq!(first_mtime, second_mtime); +} + +#[test] +fn test_source_collection_all_sources() { + let tmp = TempDir::new().unwrap(); + let src_dir = tmp.path().join("src"); + let core_dir = tmp.path().join("core"); + let variant_dir = tmp.path().join("variant"); + let build_dir = tmp.path().join("build"); + fs::create_dir_all(&src_dir).unwrap(); + fs::create_dir_all(&core_dir).unwrap(); + fs::create_dir_all(&variant_dir).unwrap(); + fs::write(src_dir.join("main.cpp"), "").unwrap(); + fs::write(core_dir.join("core.cpp"), "").unwrap(); + fs::write(variant_dir.join("variant.c"), "").unwrap(); + + let scanner = SourceScanner::new(&src_dir, &build_dir); + let collection = scanner + .scan_all(Some(&core_dir), Some(&variant_dir)) + .unwrap(); + assert_eq!(collection.sketch_sources.len(), 1); + assert_eq!(collection.core_sources.len(), 1); + assert_eq!(collection.variant_sources.len(), 1); + assert_eq!(collection.all_sources().len(), 3); +} + +#[test] +fn test_scan_sketch_sources_filtered_excludes_subdirectory() { + let (_tmp, src_dir, build_dir) = setup_project(&[ + ("main.cpp", "int main() { return 0; }"), + ("generated/skip.cpp", "void skip() {}"), + ]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner + .scan_sketch_sources_filtered(Some("+<*>\n-")) + .unwrap(); + assert_eq!(sources.len(), 1); + assert!(sources[0].ends_with("main.cpp")); +} + +#[test] +fn test_scan_sketch_sources_filtered_includes_only_selected_files() { + let (_tmp, src_dir, build_dir) = setup_project(&[ + ("main.cpp", "int main() { return 0; }"), + ("helper.cpp", "void helper() {}"), + ("sub/util.c", "void util() {}"), + ]); + let scanner = SourceScanner::new(&src_dir, &build_dir); + let sources = scanner + .scan_sketch_sources_filtered(Some("+\n+")) + .unwrap(); + assert_eq!(sources.len(), 2); + assert!(sources.iter().any(|p| p.ends_with("main.cpp"))); + assert!(sources + .iter() + .any(|p| p.ends_with("sub\\util.c") || p.ends_with("sub/util.c"))); + assert!(!sources.iter().any(|p| p.ends_with("helper.cpp"))); +} diff --git a/crates/fbuild-cli/src/daemon_client.rs b/crates/fbuild-cli/src/daemon_client.rs index 568b9d3d..27927fb1 100644 --- a/crates/fbuild-cli/src/daemon_client.rs +++ b/crates/fbuild-cli/src/daemon_client.rs @@ -949,101 +949,4 @@ fn strip_std_handle_inheritance() { } #[cfg(test)] -mod tests { - use super::{ - broker_refusal_is_fatal, daemon_cache_identity_error, DaemonAcquisition, DaemonInfoResponse, - }; - use running_process::broker::client::RefusalKind::{VersionBlocked, VersionUnsupported}; - - #[test] - fn broker_version_refusals_are_fatal() { - assert!(broker_refusal_is_fatal(Some(VersionUnsupported))); - assert!(broker_refusal_is_fatal(Some(VersionBlocked))); - } - - #[test] - fn broker_non_refusal_errors_can_fallback() { - assert!(!broker_refusal_is_fatal(None)); - } - - #[test] - fn broker_acquisition_reports_negotiated_state() { - let acquisition = DaemonAcquisition::BrokerNegotiated { - endpoint: "rp-backend".to_string(), - daemon_version: Some("2.2.29".to_string()), - }; - - assert_eq!(acquisition.mode(), "broker-negotiated"); - assert_eq!(acquisition.endpoint(), Some("rp-backend")); - assert_eq!(acquisition.daemon_version(), Some("2.2.29")); - assert_eq!(acquisition.reason(), None); - assert!(acquisition.summary().contains("version 2.2.29")); - } - - #[test] - fn direct_acquisition_reports_fallback_reason() { - let acquisition = DaemonAcquisition::DirectFallback { - reason: "broker unavailable".to_string(), - }; - - assert_eq!(acquisition.mode(), "direct-fallback"); - assert_eq!(acquisition.endpoint(), None); - assert_eq!(acquisition.daemon_version(), None); - assert_eq!(acquisition.reason(), Some("broker unavailable")); - assert!(acquisition.summary().contains("broker unavailable")); - } - - fn daemon_info_for_cache_identity( - cache_identity: Option, - cache_schema_version: Option, - ) -> DaemonInfoResponse { - DaemonInfoResponse { - status: "running".to_string(), - uptime_seconds: 1.0, - version: "2.2.29".to_string(), - pid: 123, - port: 8765, - dev_mode: fbuild_paths::is_dev_mode(), - operation_in_progress: false, - daemon_state: fbuild_core::DaemonState::Idle, - current_operation: None, - dependency_install: None, - client_count: 0, - cache_identity, - cache_schema_version, - spawner_cwd: None, - source_mtime: None, - } - } - - #[test] - fn daemon_cache_identity_accepts_current_identity() { - let identity = fbuild_paths::running_process::DaemonCacheIdentity::discover(); - let info = daemon_info_for_cache_identity( - Some(identity.label_value()), - Some(fbuild_paths::running_process::CACHE_SCHEMA_VERSION), - ); - - assert!(daemon_cache_identity_error(&info).is_none()); - } - - #[test] - fn daemon_cache_identity_rejects_missing_identity() { - let info = daemon_info_for_cache_identity( - None, - Some(fbuild_paths::running_process::CACHE_SCHEMA_VERSION), - ); - - let err = daemon_cache_identity_error(&info).expect("missing identity must fail closed"); - assert!(err.contains("cache identity")); - } - - #[test] - fn daemon_cache_identity_rejects_wrong_schema() { - let identity = fbuild_paths::running_process::DaemonCacheIdentity::discover(); - let info = daemon_info_for_cache_identity(Some(identity.label_value()), Some(u32::MAX)); - - let err = daemon_cache_identity_error(&info).expect("schema mismatch must fail closed"); - assert!(err.contains("cache schema")); - } -} +mod tests; diff --git a/crates/fbuild-cli/src/daemon_client/tests.rs b/crates/fbuild-cli/src/daemon_client/tests.rs new file mode 100644 index 00000000..aa1c576d --- /dev/null +++ b/crates/fbuild-cli/src/daemon_client/tests.rs @@ -0,0 +1,99 @@ +//! Unit tests for the parent `daemon_client` module. Extracted to keep the +//! parent file under the 1000-LOC gate (see ci.yml LOC Gate workflow). + +use super::{ + broker_refusal_is_fatal, daemon_cache_identity_error, DaemonAcquisition, DaemonInfoResponse, +}; +use running_process::broker::client::RefusalKind::{VersionBlocked, VersionUnsupported}; + +#[test] +fn broker_version_refusals_are_fatal() { + assert!(broker_refusal_is_fatal(Some(VersionUnsupported))); + assert!(broker_refusal_is_fatal(Some(VersionBlocked))); +} + +#[test] +fn broker_non_refusal_errors_can_fallback() { + assert!(!broker_refusal_is_fatal(None)); +} + +#[test] +fn broker_acquisition_reports_negotiated_state() { + let acquisition = DaemonAcquisition::BrokerNegotiated { + endpoint: "rp-backend".to_string(), + daemon_version: Some("2.2.29".to_string()), + }; + + assert_eq!(acquisition.mode(), "broker-negotiated"); + assert_eq!(acquisition.endpoint(), Some("rp-backend")); + assert_eq!(acquisition.daemon_version(), Some("2.2.29")); + assert_eq!(acquisition.reason(), None); + assert!(acquisition.summary().contains("version 2.2.29")); +} + +#[test] +fn direct_acquisition_reports_fallback_reason() { + let acquisition = DaemonAcquisition::DirectFallback { + reason: "broker unavailable".to_string(), + }; + + assert_eq!(acquisition.mode(), "direct-fallback"); + assert_eq!(acquisition.endpoint(), None); + assert_eq!(acquisition.daemon_version(), None); + assert_eq!(acquisition.reason(), Some("broker unavailable")); + assert!(acquisition.summary().contains("broker unavailable")); +} + +fn daemon_info_for_cache_identity( + cache_identity: Option, + cache_schema_version: Option, +) -> DaemonInfoResponse { + DaemonInfoResponse { + status: "running".to_string(), + uptime_seconds: 1.0, + version: "2.2.29".to_string(), + pid: 123, + port: 8765, + dev_mode: fbuild_paths::is_dev_mode(), + operation_in_progress: false, + daemon_state: fbuild_core::DaemonState::Idle, + current_operation: None, + dependency_install: None, + client_count: 0, + cache_identity, + cache_schema_version, + spawner_cwd: None, + source_mtime: None, + } +} + +#[test] +fn daemon_cache_identity_accepts_current_identity() { + let identity = fbuild_paths::running_process::DaemonCacheIdentity::discover(); + let info = daemon_info_for_cache_identity( + Some(identity.label_value()), + Some(fbuild_paths::running_process::CACHE_SCHEMA_VERSION), + ); + + assert!(daemon_cache_identity_error(&info).is_none()); +} + +#[test] +fn daemon_cache_identity_rejects_missing_identity() { + let info = daemon_info_for_cache_identity( + None, + Some(fbuild_paths::running_process::CACHE_SCHEMA_VERSION), + ); + + let err = daemon_cache_identity_error(&info).expect("missing identity must fail closed"); + assert!(err.contains("cache identity")); +} + +#[test] +fn daemon_cache_identity_rejects_wrong_schema() { + let identity = fbuild_paths::running_process::DaemonCacheIdentity::discover(); + let info = daemon_info_for_cache_identity(Some(identity.label_value()), Some(u32::MAX)); + + let err = daemon_cache_identity_error(&info).expect("schema mismatch must fail closed"); + assert!(err.contains("cache schema")); +} diff --git a/crates/fbuild-daemon/src/device_manager.rs b/crates/fbuild-daemon/src/device_manager.rs index a0b4bba3..6bbdee2a 100644 --- a/crates/fbuild-daemon/src/device_manager.rs +++ b/crates/fbuild-daemon/src/device_manager.rs @@ -684,323 +684,4 @@ impl DeviceManager { } #[cfg(test)] -mod tests { - use super::*; - - fn make_manager_with_device(port: &str) -> DeviceManager { - let mgr = DeviceManager::new(); - mgr.insert_test_device(port); - mgr - } - - #[test] - fn new_manager_has_no_devices() { - let mgr = DeviceManager::new(); - assert!(mgr.get_all_devices().is_empty()); - } - - #[test] - fn acquire_exclusive_succeeds() { - let mgr = make_manager_with_device("COM3"); - let lease = mgr - .acquire_exclusive("COM3", "client-1", "testing", false) - .unwrap(); - assert_eq!(lease.lease_type, LeaseType::Exclusive); - assert_eq!(lease.client_id, "client-1"); - } - - #[test] - fn acquire_exclusive_twice_fails() { - let mgr = make_manager_with_device("COM3"); - mgr.acquire_exclusive("COM3", "client-1", "first", false) - .unwrap(); - let result = mgr.acquire_exclusive("COM3", "client-2", "second", false); - match result.unwrap_err() { - DeviceLeaseError::ExclusiveConflict { - port, - device_id, - holder, - .. - } => { - assert_eq!(port, "COM3"); - assert_eq!(device_id, "1234:5678"); - assert_eq!(holder.client_id, "client-1"); - assert_eq!(holder.description, "first"); - } - other => panic!("expected exclusive conflict, got {other:?}"), - } - } - - #[test] - fn acquire_monitor_succeeds() { - let mgr = make_manager_with_device("COM3"); - let lease = mgr - .acquire_monitor("COM3", "client-1", "monitoring", false) - .unwrap(); - assert_eq!(lease.lease_type, LeaseType::Monitor); - } - - #[test] - fn multiple_monitor_leases_allowed() { - let mgr = make_manager_with_device("COM3"); - mgr.acquire_monitor("COM3", "client-1", "m1", false) - .unwrap(); - mgr.acquire_monitor("COM3", "client-2", "m2", false) - .unwrap(); - let state = mgr.get_device_status("COM3").unwrap(); - assert_eq!(state.monitor_count(), 2); - } - - #[test] - fn release_lease_by_id() { - let mgr = make_manager_with_device("COM3"); - let lease = mgr - .acquire_exclusive("COM3", "client-1", "test", false) - .unwrap(); - mgr.release_lease(&lease.lease_id).unwrap(); - let state = mgr.get_device_status("COM3").unwrap(); - assert!(state.exclusive_lease.is_none()); - } - - #[test] - fn release_nonexistent_lease_fails() { - let mgr = make_manager_with_device("COM3"); - assert!(mgr.release_lease("nonexistent").is_err()); - } - - #[test] - fn release_device_leases_clears_all() { - let mgr = make_manager_with_device("COM3"); - mgr.acquire_exclusive("COM3", "c1", "exc", false).unwrap(); - mgr.acquire_monitor("COM3", "c2", "mon", false).unwrap(); - let count = mgr.release_device_leases("COM3").unwrap(); - assert_eq!(count, 2); // 1 exclusive + 1 monitor - let state = mgr.get_device_status("COM3").unwrap(); - assert!(state.exclusive_lease.is_none()); - assert_eq!(state.monitor_count(), 0); - } - - #[test] - fn preempt_requires_reason() { - let mgr = make_manager_with_device("COM3"); - mgr.acquire_exclusive("COM3", "c1", "holder", false) - .unwrap(); - let result = mgr.preempt_device("COM3", "c2", ""); - assert!(result.is_err()); - assert!(result.unwrap_err().message().contains("reason is required")); - } - - #[test] - fn preempt_replaces_holder() { - let mgr = make_manager_with_device("COM3"); - mgr.acquire_exclusive("COM3", "c1", "original", false) - .unwrap(); - let (lease, preempted) = mgr.preempt_device("COM3", "c2", "urgent deploy").unwrap(); - assert_eq!(lease.client_id, "c2"); - assert_eq!(lease.lease_type, LeaseType::Exclusive); - assert_eq!(preempted.as_deref(), Some("c1")); - // Old holder should be gone - let state = mgr.get_device_status("COM3").unwrap(); - assert_eq!(state.exclusive_lease.as_ref().unwrap().client_id, "c2"); - } - - #[test] - fn device_not_found_errors() { - let mgr = DeviceManager::new(); - assert!(mgr.acquire_exclusive("COM99", "c1", "x", false).is_err()); - assert!(mgr.acquire_monitor("COM99", "c1", "x", false).is_err()); - assert!(mgr.preempt_device("COM99", "c1", "reason").is_err()); - } - - #[test] - fn disconnected_device_rejects_leases() { - let mgr = make_manager_with_device("COM3"); - { - let mut devices = mgr.devices.lock().unwrap(); - devices.get_mut("COM3").unwrap().is_connected = false; - } - assert!(mgr.acquire_exclusive("COM3", "c1", "x", false).is_err()); - assert!(mgr.acquire_monitor("COM3", "c1", "x", false).is_err()); - assert!(mgr.preempt_device("COM3", "c1", "reason").is_err()); - } - - #[test] - fn cleanup_removes_disconnected_unlocked() { - let mgr = make_manager_with_device("COM3"); - { - let mut devices = mgr.devices.lock().unwrap(); - devices.get_mut("COM3").unwrap().is_connected = false; - } - assert_eq!(mgr.cleanup_stale_devices(), 1); - assert!(mgr.get_all_devices().is_empty()); - } - - /// Calling `refresh_devices_if_stale` twice back-to-back with a - /// generous max-age must only actually run one OS-level - /// enumeration — the second call is inside the freshness window - /// and returns `false`. Regression guard for the sub-1 s warm - /// deploy path (#114 follow-up). - #[test] - fn refresh_devices_if_stale_skips_inside_window() { - let mgr = DeviceManager::new(); - assert!(mgr.refresh_devices_if_stale(std::time::Duration::from_secs(5))); - assert!(!mgr.refresh_devices_if_stale(std::time::Duration::from_secs(5))); - } - - /// An already-stale refresh window must trigger a real - /// enumeration on the next call. `Duration::ZERO` is the - /// strictest case — any elapsed time is >= 0, so only an - /// in-flight call can short-circuit (and we don't have one). - #[test] - fn refresh_devices_if_stale_reruns_when_expired() { - let mgr = DeviceManager::new(); - assert!(mgr.refresh_devices_if_stale(std::time::Duration::from_secs(5))); - assert!(mgr.refresh_devices_if_stale(std::time::Duration::ZERO)); - } - - #[test] - fn tracked_serial_lease_moves_to_new_port_on_refresh() { - let mgr = make_manager_with_device("COM3"); - mgr.acquire_exclusive("COM3", "c1", "tracked deploy", true) - .unwrap(); - - mgr.refresh_from_discovered(vec![DiscoveredDevice { - port: "COM4".to_string(), - device_id: "1234:5678".to_string(), - description: "Test Device Renumbered".to_string(), - vid: Some(0x1234), - pid: Some(0x5678), - vendor_name: Some("Test Vendor".to_string()), - product_name: Some("Test Device".to_string()), - serial_number: Some("TEST-SERIAL".to_string()), - }]); - - assert!(mgr.get_device_status("COM3").is_none()); - let moved = mgr.get_device_status("COM4").unwrap(); - assert_eq!(moved.previous_port.as_deref(), Some("COM3")); - assert_eq!( - moved.exclusive_lease.as_ref().map(|l| l.client_id.as_str()), - Some("c1") - ); - assert!( - moved.exclusive_lease.as_ref().unwrap().track_serial, - "moved lease must retain track_serial" - ); - assert_eq!( - mgr.take_recent_port_moves(), - vec![DevicePortMove { - previous_port: "COM3".to_string(), - port: "COM4".to_string(), - serial_number: Some("TEST-SERIAL".to_string()), - }] - ); - assert!( - mgr.take_recent_port_moves().is_empty(), - "taking recent moves should drain the queue" - ); - } - - #[test] - fn untracked_serial_lease_stays_on_old_disconnected_port() { - let mgr = make_manager_with_device("COM3"); - mgr.acquire_exclusive("COM3", "c1", "untracked deploy", false) - .unwrap(); - - mgr.refresh_from_discovered(vec![DiscoveredDevice { - port: "COM4".to_string(), - device_id: "1234:5678".to_string(), - description: "Test Device Renumbered".to_string(), - vid: Some(0x1234), - pid: Some(0x5678), - vendor_name: Some("Test Vendor".to_string()), - product_name: Some("Test Device".to_string()), - serial_number: Some("TEST-SERIAL".to_string()), - }]); - - let old = mgr.get_device_status("COM3").unwrap(); - assert!(!old.is_connected); - assert!(old.exclusive_lease.is_some()); - let new = mgr.get_device_status("COM4").unwrap(); - assert!(new.exclusive_lease.is_none()); - } - - #[test] - fn trusted_hash_round_trip() { - let mgr = make_manager_with_device("COM3"); - assert_eq!(mgr.trusted_firmware_hash("COM3"), None); - let h = [7u8; 32]; - mgr.set_trusted_firmware_hash("COM3", h); - assert_eq!(mgr.trusted_firmware_hash("COM3"), Some(h)); - } - - #[test] - fn trusted_hash_cleared_on_demand() { - let mgr = make_manager_with_device("COM3"); - mgr.set_trusted_firmware_hash("COM3", [1u8; 32]); - mgr.clear_trusted_firmware_hash("COM3"); - assert_eq!(mgr.trusted_firmware_hash("COM3"), None); - } - - /// Unknown port is never trusted — no panic, no fabrication of - /// device state. Regression guard: the deploy handler calls this - /// with a user-supplied port string that may not be in the - /// daemon's enumeration cache yet. - #[test] - fn trusted_hash_unknown_port_is_none() { - let mgr = DeviceManager::new(); - assert_eq!(mgr.trusted_firmware_hash("COM99"), None); - } - - /// A disconnected device must never be trusted, even if the hash - /// was previously recorded. Re-enumeration can come back with a - /// physically different board on the same port name — trust - /// across that boundary is unsafe. - #[test] - fn trusted_hash_invalid_after_disconnect() { - let mgr = make_manager_with_device("COM3"); - mgr.set_trusted_firmware_hash("COM3", [9u8; 32]); - // Simulate a disconnect that happened *after* the trust was - // recorded — exactly the condition - // `trusted_firmware_hash` must treat as "unsafe to trust". - { - let mut devices = mgr.devices.lock().unwrap(); - let state = devices.get_mut("COM3").unwrap(); - state.is_connected = false; - state.last_disconnect_at = Some(Instant::now()); - } - assert_eq!(mgr.trusted_firmware_hash("COM3"), None); - } - - /// A stale disconnect stamp from *before* the trust was set - /// (e.g. device was unplugged earlier in the session but the - /// user reconnected and we re-trusted) does NOT invalidate the - /// fresh trust. Ordering is what matters: `disconnect > set` ⇒ - /// untrusted; `set > disconnect` ⇒ trusted. - #[test] - fn trusted_hash_survives_older_disconnect_stamp() { - let mgr = make_manager_with_device("COM3"); - // Plant an old disconnect stamp first, then re-set trust - // later (what happens on a re-enumerate + fresh deploy). - { - let mut devices = mgr.devices.lock().unwrap(); - let state = devices.get_mut("COM3").unwrap(); - state.last_disconnect_at = Some(Instant::now()); - } - std::thread::sleep(std::time::Duration::from_millis(5)); - mgr.set_trusted_firmware_hash("COM3", [3u8; 32]); - assert_eq!(mgr.trusted_firmware_hash("COM3"), Some([3u8; 32])); - } - - #[test] - fn cleanup_preserves_leased_disconnected() { - let mgr = make_manager_with_device("COM3"); - mgr.acquire_exclusive("COM3", "c1", "deploy", false) - .unwrap(); - { - let mut devices = mgr.devices.lock().unwrap(); - devices.get_mut("COM3").unwrap().is_connected = false; - } - assert_eq!(mgr.cleanup_stale_devices(), 0); - assert_eq!(mgr.get_all_devices().len(), 1); - } -} +mod tests; diff --git a/crates/fbuild-daemon/src/device_manager/README.md b/crates/fbuild-daemon/src/device_manager/README.md new file mode 100644 index 00000000..0df25092 --- /dev/null +++ b/crates/fbuild-daemon/src/device_manager/README.md @@ -0,0 +1,10 @@ +# `device_manager/` + +Sibling-directory submodules for `device_manager.rs`. Today this is +just `tests.rs` — the original `#[cfg(test)] mod tests { ... }` block +was lifted out of the parent file as a separate file so the parent +stays under the 1000-LOC gate (see `.github/workflows/loc-gate.yml`). + +When growing this module, prefer adding cohesive submodules here (per +the `foo.rs → foo/.rs` split convention in the root `CLAUDE.md`) +over letting the parent file balloon back over the limit. diff --git a/crates/fbuild-daemon/src/device_manager/tests.rs b/crates/fbuild-daemon/src/device_manager/tests.rs new file mode 100644 index 00000000..0ad68c25 --- /dev/null +++ b/crates/fbuild-daemon/src/device_manager/tests.rs @@ -0,0 +1,321 @@ +//! Unit tests for the parent `device_manager` module. Extracted to keep the +//! parent file under the 1000-LOC gate (see ci.yml LOC Gate workflow). + +use super::*; + +fn make_manager_with_device(port: &str) -> DeviceManager { + let mgr = DeviceManager::new(); + mgr.insert_test_device(port); + mgr +} + +#[test] +fn new_manager_has_no_devices() { + let mgr = DeviceManager::new(); + assert!(mgr.get_all_devices().is_empty()); +} + +#[test] +fn acquire_exclusive_succeeds() { + let mgr = make_manager_with_device("COM3"); + let lease = mgr + .acquire_exclusive("COM3", "client-1", "testing", false) + .unwrap(); + assert_eq!(lease.lease_type, LeaseType::Exclusive); + assert_eq!(lease.client_id, "client-1"); +} + +#[test] +fn acquire_exclusive_twice_fails() { + let mgr = make_manager_with_device("COM3"); + mgr.acquire_exclusive("COM3", "client-1", "first", false) + .unwrap(); + let result = mgr.acquire_exclusive("COM3", "client-2", "second", false); + match result.unwrap_err() { + DeviceLeaseError::ExclusiveConflict { + port, + device_id, + holder, + .. + } => { + assert_eq!(port, "COM3"); + assert_eq!(device_id, "1234:5678"); + assert_eq!(holder.client_id, "client-1"); + assert_eq!(holder.description, "first"); + } + other => panic!("expected exclusive conflict, got {other:?}"), + } +} + +#[test] +fn acquire_monitor_succeeds() { + let mgr = make_manager_with_device("COM3"); + let lease = mgr + .acquire_monitor("COM3", "client-1", "monitoring", false) + .unwrap(); + assert_eq!(lease.lease_type, LeaseType::Monitor); +} + +#[test] +fn multiple_monitor_leases_allowed() { + let mgr = make_manager_with_device("COM3"); + mgr.acquire_monitor("COM3", "client-1", "m1", false) + .unwrap(); + mgr.acquire_monitor("COM3", "client-2", "m2", false) + .unwrap(); + let state = mgr.get_device_status("COM3").unwrap(); + assert_eq!(state.monitor_count(), 2); +} + +#[test] +fn release_lease_by_id() { + let mgr = make_manager_with_device("COM3"); + let lease = mgr + .acquire_exclusive("COM3", "client-1", "test", false) + .unwrap(); + mgr.release_lease(&lease.lease_id).unwrap(); + let state = mgr.get_device_status("COM3").unwrap(); + assert!(state.exclusive_lease.is_none()); +} + +#[test] +fn release_nonexistent_lease_fails() { + let mgr = make_manager_with_device("COM3"); + assert!(mgr.release_lease("nonexistent").is_err()); +} + +#[test] +fn release_device_leases_clears_all() { + let mgr = make_manager_with_device("COM3"); + mgr.acquire_exclusive("COM3", "c1", "exc", false).unwrap(); + mgr.acquire_monitor("COM3", "c2", "mon", false).unwrap(); + let count = mgr.release_device_leases("COM3").unwrap(); + assert_eq!(count, 2); // 1 exclusive + 1 monitor + let state = mgr.get_device_status("COM3").unwrap(); + assert!(state.exclusive_lease.is_none()); + assert_eq!(state.monitor_count(), 0); +} + +#[test] +fn preempt_requires_reason() { + let mgr = make_manager_with_device("COM3"); + mgr.acquire_exclusive("COM3", "c1", "holder", false) + .unwrap(); + let result = mgr.preempt_device("COM3", "c2", ""); + assert!(result.is_err()); + assert!(result.unwrap_err().message().contains("reason is required")); +} + +#[test] +fn preempt_replaces_holder() { + let mgr = make_manager_with_device("COM3"); + mgr.acquire_exclusive("COM3", "c1", "original", false) + .unwrap(); + let (lease, preempted) = mgr.preempt_device("COM3", "c2", "urgent deploy").unwrap(); + assert_eq!(lease.client_id, "c2"); + assert_eq!(lease.lease_type, LeaseType::Exclusive); + assert_eq!(preempted.as_deref(), Some("c1")); + // Old holder should be gone + let state = mgr.get_device_status("COM3").unwrap(); + assert_eq!(state.exclusive_lease.as_ref().unwrap().client_id, "c2"); +} + +#[test] +fn device_not_found_errors() { + let mgr = DeviceManager::new(); + assert!(mgr.acquire_exclusive("COM99", "c1", "x", false).is_err()); + assert!(mgr.acquire_monitor("COM99", "c1", "x", false).is_err()); + assert!(mgr.preempt_device("COM99", "c1", "reason").is_err()); +} + +#[test] +fn disconnected_device_rejects_leases() { + let mgr = make_manager_with_device("COM3"); + { + let mut devices = mgr.devices.lock().unwrap(); + devices.get_mut("COM3").unwrap().is_connected = false; + } + assert!(mgr.acquire_exclusive("COM3", "c1", "x", false).is_err()); + assert!(mgr.acquire_monitor("COM3", "c1", "x", false).is_err()); + assert!(mgr.preempt_device("COM3", "c1", "reason").is_err()); +} + +#[test] +fn cleanup_removes_disconnected_unlocked() { + let mgr = make_manager_with_device("COM3"); + { + let mut devices = mgr.devices.lock().unwrap(); + devices.get_mut("COM3").unwrap().is_connected = false; + } + assert_eq!(mgr.cleanup_stale_devices(), 1); + assert!(mgr.get_all_devices().is_empty()); +} + +/// Calling `refresh_devices_if_stale` twice back-to-back with a +/// generous max-age must only actually run one OS-level +/// enumeration — the second call is inside the freshness window +/// and returns `false`. Regression guard for the sub-1 s warm +/// deploy path (#114 follow-up). +#[test] +fn refresh_devices_if_stale_skips_inside_window() { + let mgr = DeviceManager::new(); + assert!(mgr.refresh_devices_if_stale(std::time::Duration::from_secs(5))); + assert!(!mgr.refresh_devices_if_stale(std::time::Duration::from_secs(5))); +} + +/// An already-stale refresh window must trigger a real +/// enumeration on the next call. `Duration::ZERO` is the +/// strictest case — any elapsed time is >= 0, so only an +/// in-flight call can short-circuit (and we don't have one). +#[test] +fn refresh_devices_if_stale_reruns_when_expired() { + let mgr = DeviceManager::new(); + assert!(mgr.refresh_devices_if_stale(std::time::Duration::from_secs(5))); + assert!(mgr.refresh_devices_if_stale(std::time::Duration::ZERO)); +} + +#[test] +fn tracked_serial_lease_moves_to_new_port_on_refresh() { + let mgr = make_manager_with_device("COM3"); + mgr.acquire_exclusive("COM3", "c1", "tracked deploy", true) + .unwrap(); + + mgr.refresh_from_discovered(vec![DiscoveredDevice { + port: "COM4".to_string(), + device_id: "1234:5678".to_string(), + description: "Test Device Renumbered".to_string(), + vid: Some(0x1234), + pid: Some(0x5678), + vendor_name: Some("Test Vendor".to_string()), + product_name: Some("Test Device".to_string()), + serial_number: Some("TEST-SERIAL".to_string()), + }]); + + assert!(mgr.get_device_status("COM3").is_none()); + let moved = mgr.get_device_status("COM4").unwrap(); + assert_eq!(moved.previous_port.as_deref(), Some("COM3")); + assert_eq!( + moved.exclusive_lease.as_ref().map(|l| l.client_id.as_str()), + Some("c1") + ); + assert!( + moved.exclusive_lease.as_ref().unwrap().track_serial, + "moved lease must retain track_serial" + ); + assert_eq!( + mgr.take_recent_port_moves(), + vec![DevicePortMove { + previous_port: "COM3".to_string(), + port: "COM4".to_string(), + serial_number: Some("TEST-SERIAL".to_string()), + }] + ); + assert!( + mgr.take_recent_port_moves().is_empty(), + "taking recent moves should drain the queue" + ); +} + +#[test] +fn untracked_serial_lease_stays_on_old_disconnected_port() { + let mgr = make_manager_with_device("COM3"); + mgr.acquire_exclusive("COM3", "c1", "untracked deploy", false) + .unwrap(); + + mgr.refresh_from_discovered(vec![DiscoveredDevice { + port: "COM4".to_string(), + device_id: "1234:5678".to_string(), + description: "Test Device Renumbered".to_string(), + vid: Some(0x1234), + pid: Some(0x5678), + vendor_name: Some("Test Vendor".to_string()), + product_name: Some("Test Device".to_string()), + serial_number: Some("TEST-SERIAL".to_string()), + }]); + + let old = mgr.get_device_status("COM3").unwrap(); + assert!(!old.is_connected); + assert!(old.exclusive_lease.is_some()); + let new = mgr.get_device_status("COM4").unwrap(); + assert!(new.exclusive_lease.is_none()); +} + +#[test] +fn trusted_hash_round_trip() { + let mgr = make_manager_with_device("COM3"); + assert_eq!(mgr.trusted_firmware_hash("COM3"), None); + let h = [7u8; 32]; + mgr.set_trusted_firmware_hash("COM3", h); + assert_eq!(mgr.trusted_firmware_hash("COM3"), Some(h)); +} + +#[test] +fn trusted_hash_cleared_on_demand() { + let mgr = make_manager_with_device("COM3"); + mgr.set_trusted_firmware_hash("COM3", [1u8; 32]); + mgr.clear_trusted_firmware_hash("COM3"); + assert_eq!(mgr.trusted_firmware_hash("COM3"), None); +} + +/// Unknown port is never trusted — no panic, no fabrication of +/// device state. Regression guard: the deploy handler calls this +/// with a user-supplied port string that may not be in the +/// daemon's enumeration cache yet. +#[test] +fn trusted_hash_unknown_port_is_none() { + let mgr = DeviceManager::new(); + assert_eq!(mgr.trusted_firmware_hash("COM99"), None); +} + +/// A disconnected device must never be trusted, even if the hash +/// was previously recorded. Re-enumeration can come back with a +/// physically different board on the same port name — trust +/// across that boundary is unsafe. +#[test] +fn trusted_hash_invalid_after_disconnect() { + let mgr = make_manager_with_device("COM3"); + mgr.set_trusted_firmware_hash("COM3", [9u8; 32]); + // Simulate a disconnect that happened *after* the trust was + // recorded — exactly the condition + // `trusted_firmware_hash` must treat as "unsafe to trust". + { + let mut devices = mgr.devices.lock().unwrap(); + let state = devices.get_mut("COM3").unwrap(); + state.is_connected = false; + state.last_disconnect_at = Some(Instant::now()); + } + assert_eq!(mgr.trusted_firmware_hash("COM3"), None); +} + +/// A stale disconnect stamp from *before* the trust was set +/// (e.g. device was unplugged earlier in the session but the +/// user reconnected and we re-trusted) does NOT invalidate the +/// fresh trust. Ordering is what matters: `disconnect > set` ⇒ +/// untrusted; `set > disconnect` ⇒ trusted. +#[test] +fn trusted_hash_survives_older_disconnect_stamp() { + let mgr = make_manager_with_device("COM3"); + // Plant an old disconnect stamp first, then re-set trust + // later (what happens on a re-enumerate + fresh deploy). + { + let mut devices = mgr.devices.lock().unwrap(); + let state = devices.get_mut("COM3").unwrap(); + state.last_disconnect_at = Some(Instant::now()); + } + std::thread::sleep(std::time::Duration::from_millis(5)); + mgr.set_trusted_firmware_hash("COM3", [3u8; 32]); + assert_eq!(mgr.trusted_firmware_hash("COM3"), Some([3u8; 32])); +} + +#[test] +fn cleanup_preserves_leased_disconnected() { + let mgr = make_manager_with_device("COM3"); + mgr.acquire_exclusive("COM3", "c1", "deploy", false) + .unwrap(); + { + let mut devices = mgr.devices.lock().unwrap(); + devices.get_mut("COM3").unwrap().is_connected = false; + } + assert_eq!(mgr.cleanup_stale_devices(), 0); + assert_eq!(mgr.get_all_devices().len(), 1); +} diff --git a/crates/fbuild-daemon/src/handlers/websockets.rs b/crates/fbuild-daemon/src/handlers/websockets.rs index f521a0a2..4dd4fa02 100644 --- a/crates/fbuild-daemon/src/handlers/websockets.rs +++ b/crates/fbuild-daemon/src/handlers/websockets.rs @@ -430,18 +430,20 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc) { .await { Ok(n) => { - let _ = out_tx_inbound.send(SerialServerMessage::WriteAck { - success: true, - bytes_written: n, - message: None, - }); + let _ = + out_tx_inbound.send(SerialServerMessage::WriteAck { + success: true, + bytes_written: n, + message: None, + }); } Err(e) => { - let _ = out_tx_inbound.send(SerialServerMessage::WriteAck { - success: false, - bytes_written: 0, - message: Some(format!("write error: {}", e)), - }); + let _ = + out_tx_inbound.send(SerialServerMessage::WriteAck { + success: false, + bytes_written: 0, + message: Some(format!("write error: {}", e)), + }); tracing::warn!( client_id = %client_id_owned, port = %port_owned, @@ -479,9 +481,8 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc) { // honest (the client's view of "buffered" // lines lives in the daemon -> client // mpsc queue, which is unbounded). - let _ = out_tx_inbound.send(SerialServerMessage::InWaiting { - count: 0, - }); + let _ = out_tx_inbound + .send(SerialServerMessage::InWaiting { count: 0 }); } Ok(_) => {} Err(e) => { diff --git a/crates/fbuild-serial/src/manager.rs b/crates/fbuild-serial/src/manager.rs index b851fe08..56bc77d7 100644 --- a/crates/fbuild-serial/src/manager.rs +++ b/crates/fbuild-serial/src/manager.rs @@ -969,512 +969,4 @@ impl Default for SharedSerialManager { } #[cfg(test)] -mod tests { - use super::*; - use serialport::{ClearBuffer, DataBits, FlowControl, Parity, StopBits}; - use std::io::{Read, Write}; - - #[derive(Clone)] - struct FakeSerialPort { - name: String, - writes: Arc>>, - } - - impl FakeSerialPort { - fn new(name: &str) -> (Self, Arc>>) { - let writes = Arc::new(std::sync::Mutex::new(Vec::new())); - ( - Self { - name: name.to_string(), - writes: Arc::clone(&writes), - }, - writes, - ) - } - } - - impl Read for FakeSerialPort { - fn read(&mut self, _buf: &mut [u8]) -> std::io::Result { - Err(std::io::Error::new(std::io::ErrorKind::TimedOut, "no data")) - } - } - - impl Write for FakeSerialPort { - fn write(&mut self, buf: &[u8]) -> std::io::Result { - self.writes.lock().unwrap().extend_from_slice(buf); - Ok(buf.len()) - } - - fn flush(&mut self) -> std::io::Result<()> { - Ok(()) - } - } - - impl serialport::SerialPort for FakeSerialPort { - fn name(&self) -> Option { - Some(self.name.clone()) - } - fn baud_rate(&self) -> serialport::Result { - Ok(115200) - } - fn data_bits(&self) -> serialport::Result { - Ok(DataBits::Eight) - } - fn flow_control(&self) -> serialport::Result { - Ok(FlowControl::None) - } - fn parity(&self) -> serialport::Result { - Ok(Parity::None) - } - fn stop_bits(&self) -> serialport::Result { - Ok(StopBits::One) - } - fn timeout(&self) -> Duration { - Duration::from_millis(100) - } - fn set_baud_rate(&mut self, _baud_rate: u32) -> serialport::Result<()> { - Ok(()) - } - fn set_data_bits(&mut self, _data_bits: DataBits) -> serialport::Result<()> { - Ok(()) - } - fn set_flow_control(&mut self, _flow_control: FlowControl) -> serialport::Result<()> { - Ok(()) - } - fn set_parity(&mut self, _parity: Parity) -> serialport::Result<()> { - Ok(()) - } - fn set_stop_bits(&mut self, _stop_bits: StopBits) -> serialport::Result<()> { - Ok(()) - } - fn set_timeout(&mut self, _timeout: Duration) -> serialport::Result<()> { - Ok(()) - } - fn write_request_to_send(&mut self, _level: bool) -> serialport::Result<()> { - Ok(()) - } - fn write_data_terminal_ready(&mut self, _level: bool) -> serialport::Result<()> { - Ok(()) - } - fn read_clear_to_send(&mut self) -> serialport::Result { - Ok(true) - } - fn read_data_set_ready(&mut self) -> serialport::Result { - Ok(true) - } - fn read_ring_indicator(&mut self) -> serialport::Result { - Ok(false) - } - fn read_carrier_detect(&mut self) -> serialport::Result { - Ok(true) - } - fn bytes_to_read(&self) -> serialport::Result { - Ok(0) - } - fn bytes_to_write(&self) -> serialport::Result { - Ok(0) - } - fn clear(&self, _buffer_to_clear: ClearBuffer) -> serialport::Result<()> { - Ok(()) - } - fn try_clone(&self) -> serialport::Result> { - Ok(Box::new(self.clone())) - } - fn set_break(&self) -> serialport::Result<()> { - Ok(()) - } - fn clear_break(&self) -> serialport::Result<()> { - Ok(()) - } - } - - /// TDD red→green for ISSUES.md "Issue C": calling `open_port` against a - /// definitely-nonexistent port must NOT block other tokio tasks on the - /// same multi-thread runtime. Before the spawn_blocking fix, the - /// synchronous `serialport::open()` call held one of the worker threads - /// for the full retry budget; with only one worker, a concurrently - /// scheduled task could not run until the open finished. - /// - /// We use a 1-worker multi-thread runtime to make the regression - /// observable: with the fix, the keepalive task runs while the open - /// retries are blocked on a *blocking-pool* thread; without the fix, - /// the open call hogs the only worker and the keepalive ticks never - /// fire until the retries time out (~15s on Windows). - #[test] - fn open_port_does_not_starve_runtime_workers() { - let rt = tokio::runtime::Builder::new_multi_thread() - .worker_threads(1) - .enable_all() - .build() - .expect("build runtime"); - - rt.block_on(async { - let mgr = Arc::new(SharedSerialManager::new()); - let mgr_open = Arc::clone(&mgr); - - // Pick a port name that cannot exist on any platform so the - // open call always fails and retries through the full schedule. - // Using a very long invalid name avoids the slim chance of - // matching an actual /dev/ttyUSB* on Linux CI runners. - let bogus_port = "FBUILD_TEST_NONEXISTENT_PORT_xyz_zzz".to_string(); - - let open_task = tokio::spawn(async move { - let _ = mgr_open - .open_port(&bogus_port, 115200, "test_client", None) - .await; - }); - - // Concurrent keepalive: should tick at least 5 times (5 × 50ms - // = 250ms) within the first second of the open retries. With - // the bug present, this counter would still be 0 because the - // single worker is blocked inside `serialport::open()`. - let ticks = Arc::new(std::sync::atomic::AtomicUsize::new(0)); - let ticks_clone = Arc::clone(&ticks); - let keepalive = tokio::spawn(async move { - for _ in 0..20 { - tokio::time::sleep(Duration::from_millis(50)).await; - ticks_clone.fetch_add(1, Ordering::Relaxed); - } - }); - - // Wait for the keepalive to finish (1s). - let _ = tokio::time::timeout(Duration::from_secs(3), keepalive).await; - let observed = ticks.load(Ordering::Relaxed); - - // Abort the open task — we don't care about its result, only - // that it didn't starve the runtime. - open_task.abort(); - - assert!( - observed >= 5, - "concurrent task ticked {} times in 1s while open_port \ - was retrying — runtime worker is starved (Issue C \ - regression: serialport::open() must run via spawn_blocking)", - observed - ); - }); - } - - /// Regression guard for FastLED/fbuild#51: `attach_reader` used to - /// insert `client_id` into `session.reader_client_ids` even when the - /// broadcaster was missing (returning `None`). That left a dangling - /// reader id that kept `has_clients()` true forever, blocking - /// self-eviction and leaving `fbuild-daemon.exe` resident after the - /// autoresearch session ended. - /// - /// Contract: if `attach_reader` returns `None`, no session state may - /// be mutated. - #[test] - fn attach_reader_missing_broadcaster_does_not_mutate_session_state() { - let mgr = SharedSerialManager::new(); - let port = "COM_TEST_NO_BROADCASTER"; - let client = "client-1"; - - // Insert a bare session without a broadcaster — simulates the - // pathological "half-set-up" state. - mgr.sessions.insert( - port.to_string(), - super::SerialSession { - port: port.to_string(), - baud_rate: 115200, - is_open: false, - writer_client_id: None, - reader_client_ids: Default::default(), - output_buffer: Default::default(), - total_bytes_read: 0, - total_bytes_written: 0, - started_at: 0.0, - owner_client_id: None, - elf_path: None, - serial_handle: None, - reader_handle: None, - stop_flag: Arc::new(std::sync::atomic::AtomicBool::new(false)), - }, - ); - - let result = mgr.attach_reader(port, client); - assert!( - result.is_none(), - "attach_reader must return None when broadcaster is absent" - ); - - let leaked = mgr - .sessions - .get(port) - .map(|s| s.reader_client_ids.contains(client)) - .unwrap_or(false); - assert!( - !leaked, - "attach_reader must not mutate reader_client_ids when it \ - returns None — regression of FastLED/fbuild#51 where the \ - leaked id kept has_clients() true forever" - ); - } - - /// Regression guard for FastLED/fbuild#531: after a timeout/halt monitor - /// session ends, the HTTP `monitor` handler detaches its reader and then - /// closes the port when no clients remain, releasing the OS serial handle - /// so a follow-up pyserial/esptool open of the same port succeeds without - /// `fbuild daemon stop`. This locks in the manager contract that the - /// handler relies on: detach drops the last client, and close removes the - /// session (and its serial handle) from the manager entirely. - #[tokio::test] - async fn detach_then_close_releases_port_for_lone_monitor() { - let mgr = SharedSerialManager::new(); - let port = "COM_TEST_531"; - let client = "monitor-client"; - - // Simulate an open, single-reader monitor session (the timeout path): - // a broadcaster is present and one reader is attached. - let (tx, _rx) = broadcast::channel(BROADCAST_CHANNEL_SIZE); - mgr.broadcasters.insert(port.to_string(), tx); - let mut readers = std::collections::HashSet::new(); - readers.insert(client.to_string()); - mgr.sessions.insert( - port.to_string(), - super::SerialSession { - port: port.to_string(), - baud_rate: 115200, - is_open: true, - writer_client_id: None, - reader_client_ids: readers, - output_buffer: Default::default(), - total_bytes_read: 0, - total_bytes_written: 0, - started_at: 0.0, - owner_client_id: Some(client.to_string()), - elf_path: None, - serial_handle: None, - reader_handle: None, - stop_flag: Arc::new(std::sync::atomic::AtomicBool::new(false)), - }, - ); - - assert!( - mgr.has_clients(port), - "precondition: the lone monitor reader keeps the port busy" - ); - - // Mirror the handler cleanup sequence. - mgr.detach_reader(port, client); - assert!( - !mgr.has_clients(port), - "after the lone monitor detaches, no clients should remain" - ); - mgr.close_port(port, client).await.expect("close_port"); - - // The session (and its serial handle) must be gone — the OS port is - // released, so a non-fbuild client can reopen it. - assert!( - mgr.sessions.get(port).is_none(), - "close_port must remove the session so the OS handle is released \ - (regression of FastLED/fbuild#531)" - ); - assert!( - mgr.broadcasters.get(port).is_none(), - "close_port must also drop the broadcaster for the released port" - ); - } - - #[tokio::test] - async fn grace_close_removes_idle_port_after_delay() { - let mgr = Arc::new(SharedSerialManager::new()); - let port = "COM_TEST_GRACE_CLOSE"; - let client = "monitor-client"; - - mgr.sessions.insert( - port.to_string(), - super::SerialSession { - port: port.to_string(), - baud_rate: 115200, - is_open: true, - writer_client_id: None, - reader_client_ids: Default::default(), - output_buffer: Default::default(), - total_bytes_read: 0, - total_bytes_written: 0, - started_at: 0.0, - owner_client_id: Some(client.to_string()), - elf_path: None, - serial_handle: None, - reader_handle: None, - stop_flag: Arc::new(std::sync::atomic::AtomicBool::new(false)), - }, - ); - - assert!(mgr.close_port_after_grace_if_idle(port, client, Duration::from_millis(10))); - tokio::time::sleep(Duration::from_millis(50)).await; - - assert!( - mgr.sessions.get(port).is_none(), - "idle grace close should physically release the port after delay" - ); - } - - #[tokio::test] - async fn grace_close_is_canceled_by_new_reader() { - let mgr = Arc::new(SharedSerialManager::new()); - let port = "COM_TEST_GRACE_CANCEL"; - let client = "monitor-client"; - let next_client = "next-client"; - - let (tx, _rx) = broadcast::channel(BROADCAST_CHANNEL_SIZE); - mgr.broadcasters.insert(port.to_string(), tx); - mgr.sessions.insert( - port.to_string(), - super::SerialSession { - port: port.to_string(), - baud_rate: 115200, - is_open: true, - writer_client_id: None, - reader_client_ids: Default::default(), - output_buffer: Default::default(), - total_bytes_read: 0, - total_bytes_written: 0, - started_at: 0.0, - owner_client_id: Some(client.to_string()), - elf_path: None, - serial_handle: None, - reader_handle: None, - stop_flag: Arc::new(std::sync::atomic::AtomicBool::new(false)), - }, - ); - - assert!(mgr.close_port_after_grace_if_idle(port, client, Duration::from_millis(25))); - let rx = mgr.attach_reader(port, next_client); - assert!( - rx.is_some(), - "new reader should attach during pending close grace window" - ); - - tokio::time::sleep(Duration::from_millis(75)).await; - - assert!( - mgr.sessions.get(port).is_some(), - "new reader activity should cancel the pending physical close" - ); - assert!(mgr.has_clients(port)); - } - - #[test] - fn notify_port_renumbered_broadcasts_events_to_old_port() { - let mgr = SharedSerialManager::new(); - let old_port = "COM21"; - let new_port = "COM20"; - let (tx, mut rx) = broadcast::channel(BROADCAST_CHANNEL_SIZE); - mgr.broadcasters.insert(old_port.to_string(), tx); - - assert!(mgr.notify_port_renumbered( - old_port, - new_port, - "tracked_serial_move", - Some("15821020".to_string()) - )); - - assert_eq!( - rx.try_recv().unwrap(), - SerialStreamEvent::PortRenumbered { - port: old_port.to_string(), - new_port: new_port.to_string(), - reason: "tracked_serial_move".to_string(), - serial: Some("15821020".to_string()), - } - ); - assert_eq!( - rx.try_recv().unwrap(), - SerialStreamEvent::PortReattached { - port: new_port.to_string(), - previous_port: old_port.to_string(), - } - ); - } - - #[tokio::test] - async fn rebind_preserves_session_and_routes_writes_to_new_handle() { - let mgr = SharedSerialManager::new(); - let old_port = "COM21"; - let new_port = "COM20"; - let writer = "writer-client"; - let reader = "reader-client"; - let (tx, mut rx) = broadcast::channel(BROADCAST_CHANNEL_SIZE); - mgr.broadcasters.insert(old_port.to_string(), tx); - mgr.output_buffers.insert( - old_port.to_string(), - Arc::new(PortOutputBuffer { - buffer: std::sync::Mutex::new(VecDeque::with_capacity(OUTPUT_BUFFER_CAP)), - total_bytes_read: std::sync::atomic::AtomicU64::new(0), - }), - ); - let (old_fake, _old_writes) = FakeSerialPort::new(old_port); - let mut readers = std::collections::HashSet::new(); - readers.insert(reader.to_string()); - mgr.sessions.insert( - old_port.to_string(), - SerialSession { - port: old_port.to_string(), - baud_rate: 115200, - is_open: true, - writer_client_id: Some(writer.to_string()), - reader_client_ids: readers, - output_buffer: Default::default(), - total_bytes_read: 0, - total_bytes_written: 0, - started_at: 0.0, - owner_client_id: Some(writer.to_string()), - elf_path: None, - serial_handle: Some(Arc::new(Mutex::new(Box::new(old_fake)))), - reader_handle: None, - stop_flag: Arc::new(AtomicBool::new(false)), - }, - ); - let (new_fake, new_writes) = FakeSerialPort::new(new_port); - - assert!(mgr - .rebind_port_session_to_handle( - old_port, - new_port, - Arc::new(Mutex::new(Box::new(new_fake))), - "tracked_serial_move", - Some("15821020".to_string()), - ) - .await - .unwrap()); - - let session = mgr.sessions.get(old_port).expect("logical session remains"); - assert_eq!(session.port, new_port); - assert_eq!(session.writer_client_id.as_deref(), Some(writer)); - assert!(session.reader_client_ids.contains(reader)); - drop(session); - assert_eq!(mgr.reader_count(new_port), 1); - assert!(mgr.has_clients(new_port)); - - mgr.write_to_port(old_port, b"old-logical", writer) - .await - .unwrap(); - mgr.write_to_port(new_port, b"new-alias", writer) - .await - .unwrap(); - assert_eq!(&*new_writes.lock().unwrap(), b"old-logicalnew-alias"); - - assert_eq!( - rx.try_recv().unwrap(), - SerialStreamEvent::PortRenumbered { - port: old_port.to_string(), - new_port: new_port.to_string(), - reason: "tracked_serial_move".to_string(), - serial: Some("15821020".to_string()), - } - ); - assert_eq!( - rx.try_recv().unwrap(), - SerialStreamEvent::PortReattached { - port: new_port.to_string(), - previous_port: old_port.to_string(), - } - ); - - mgr.close_port(new_port, "test").await.unwrap(); - assert!(mgr.sessions.get(old_port).is_none()); - assert!(mgr.port_aliases.get(new_port).is_none()); - } -} +mod tests; diff --git a/crates/fbuild-serial/src/manager/README.md b/crates/fbuild-serial/src/manager/README.md new file mode 100644 index 00000000..122c371d --- /dev/null +++ b/crates/fbuild-serial/src/manager/README.md @@ -0,0 +1,12 @@ +# `manager/` + +Sibling-directory submodules for `manager.rs` (the `SharedSerialManager` +implementation). Today this is just `tests.rs` — the original +`#[cfg(test)] mod tests { ... }` block was lifted out of the parent +file as a separate file so the parent stays under the 1000-LOC gate +(see `.github/workflows/loc-gate.yml`). + +When growing this module, prefer cohesive per-domain submodules +(`session.rs`, `readers.rs`, `preemption.rs`, …) over letting the +parent file balloon back over the limit. The split convention is +documented in the root `CLAUDE.md`. diff --git a/crates/fbuild-serial/src/manager/tests.rs b/crates/fbuild-serial/src/manager/tests.rs new file mode 100644 index 00000000..08a72872 --- /dev/null +++ b/crates/fbuild-serial/src/manager/tests.rs @@ -0,0 +1,510 @@ +//! Unit tests for the parent `manager` module. Extracted to keep the +//! parent file under the 1000-LOC gate (see ci.yml LOC Gate workflow). + +use super::*; +use serialport::{ClearBuffer, DataBits, FlowControl, Parity, StopBits}; +use std::io::{Read, Write}; + +#[derive(Clone)] +struct FakeSerialPort { + name: String, + writes: Arc>>, +} + +impl FakeSerialPort { + fn new(name: &str) -> (Self, Arc>>) { + let writes = Arc::new(std::sync::Mutex::new(Vec::new())); + ( + Self { + name: name.to_string(), + writes: Arc::clone(&writes), + }, + writes, + ) + } +} + +impl Read for FakeSerialPort { + fn read(&mut self, _buf: &mut [u8]) -> std::io::Result { + Err(std::io::Error::new(std::io::ErrorKind::TimedOut, "no data")) + } +} + +impl Write for FakeSerialPort { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.writes.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +impl serialport::SerialPort for FakeSerialPort { + fn name(&self) -> Option { + Some(self.name.clone()) + } + fn baud_rate(&self) -> serialport::Result { + Ok(115200) + } + fn data_bits(&self) -> serialport::Result { + Ok(DataBits::Eight) + } + fn flow_control(&self) -> serialport::Result { + Ok(FlowControl::None) + } + fn parity(&self) -> serialport::Result { + Ok(Parity::None) + } + fn stop_bits(&self) -> serialport::Result { + Ok(StopBits::One) + } + fn timeout(&self) -> Duration { + Duration::from_millis(100) + } + fn set_baud_rate(&mut self, _baud_rate: u32) -> serialport::Result<()> { + Ok(()) + } + fn set_data_bits(&mut self, _data_bits: DataBits) -> serialport::Result<()> { + Ok(()) + } + fn set_flow_control(&mut self, _flow_control: FlowControl) -> serialport::Result<()> { + Ok(()) + } + fn set_parity(&mut self, _parity: Parity) -> serialport::Result<()> { + Ok(()) + } + fn set_stop_bits(&mut self, _stop_bits: StopBits) -> serialport::Result<()> { + Ok(()) + } + fn set_timeout(&mut self, _timeout: Duration) -> serialport::Result<()> { + Ok(()) + } + fn write_request_to_send(&mut self, _level: bool) -> serialport::Result<()> { + Ok(()) + } + fn write_data_terminal_ready(&mut self, _level: bool) -> serialport::Result<()> { + Ok(()) + } + fn read_clear_to_send(&mut self) -> serialport::Result { + Ok(true) + } + fn read_data_set_ready(&mut self) -> serialport::Result { + Ok(true) + } + fn read_ring_indicator(&mut self) -> serialport::Result { + Ok(false) + } + fn read_carrier_detect(&mut self) -> serialport::Result { + Ok(true) + } + fn bytes_to_read(&self) -> serialport::Result { + Ok(0) + } + fn bytes_to_write(&self) -> serialport::Result { + Ok(0) + } + fn clear(&self, _buffer_to_clear: ClearBuffer) -> serialport::Result<()> { + Ok(()) + } + fn try_clone(&self) -> serialport::Result> { + Ok(Box::new(self.clone())) + } + fn set_break(&self) -> serialport::Result<()> { + Ok(()) + } + fn clear_break(&self) -> serialport::Result<()> { + Ok(()) + } +} + +/// TDD red→green for ISSUES.md "Issue C": calling `open_port` against a +/// definitely-nonexistent port must NOT block other tokio tasks on the +/// same multi-thread runtime. Before the spawn_blocking fix, the +/// synchronous `serialport::open()` call held one of the worker threads +/// for the full retry budget; with only one worker, a concurrently +/// scheduled task could not run until the open finished. +/// +/// We use a 1-worker multi-thread runtime to make the regression +/// observable: with the fix, the keepalive task runs while the open +/// retries are blocked on a *blocking-pool* thread; without the fix, +/// the open call hogs the only worker and the keepalive ticks never +/// fire until the retries time out (~15s on Windows). +#[test] +fn open_port_does_not_starve_runtime_workers() { + let rt = tokio::runtime::Builder::new_multi_thread() + .worker_threads(1) + .enable_all() + .build() + .expect("build runtime"); + + rt.block_on(async { + let mgr = Arc::new(SharedSerialManager::new()); + let mgr_open = Arc::clone(&mgr); + + // Pick a port name that cannot exist on any platform so the + // open call always fails and retries through the full schedule. + // Using a very long invalid name avoids the slim chance of + // matching an actual /dev/ttyUSB* on Linux CI runners. + let bogus_port = "FBUILD_TEST_NONEXISTENT_PORT_xyz_zzz".to_string(); + + let open_task = tokio::spawn(async move { + let _ = mgr_open + .open_port(&bogus_port, 115200, "test_client", None) + .await; + }); + + // Concurrent keepalive: should tick at least 5 times (5 × 50ms + // = 250ms) within the first second of the open retries. With + // the bug present, this counter would still be 0 because the + // single worker is blocked inside `serialport::open()`. + let ticks = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let ticks_clone = Arc::clone(&ticks); + let keepalive = tokio::spawn(async move { + for _ in 0..20 { + tokio::time::sleep(Duration::from_millis(50)).await; + ticks_clone.fetch_add(1, Ordering::Relaxed); + } + }); + + // Wait for the keepalive to finish (1s). + let _ = tokio::time::timeout(Duration::from_secs(3), keepalive).await; + let observed = ticks.load(Ordering::Relaxed); + + // Abort the open task — we don't care about its result, only + // that it didn't starve the runtime. + open_task.abort(); + + assert!( + observed >= 5, + "concurrent task ticked {} times in 1s while open_port \ + was retrying — runtime worker is starved (Issue C \ + regression: serialport::open() must run via spawn_blocking)", + observed + ); + }); +} + +/// Regression guard for FastLED/fbuild#51: `attach_reader` used to +/// insert `client_id` into `session.reader_client_ids` even when the +/// broadcaster was missing (returning `None`). That left a dangling +/// reader id that kept `has_clients()` true forever, blocking +/// self-eviction and leaving `fbuild-daemon.exe` resident after the +/// autoresearch session ended. +/// +/// Contract: if `attach_reader` returns `None`, no session state may +/// be mutated. +#[test] +fn attach_reader_missing_broadcaster_does_not_mutate_session_state() { + let mgr = SharedSerialManager::new(); + let port = "COM_TEST_NO_BROADCASTER"; + let client = "client-1"; + + // Insert a bare session without a broadcaster — simulates the + // pathological "half-set-up" state. + mgr.sessions.insert( + port.to_string(), + super::SerialSession { + port: port.to_string(), + baud_rate: 115200, + is_open: false, + writer_client_id: None, + reader_client_ids: Default::default(), + output_buffer: Default::default(), + total_bytes_read: 0, + total_bytes_written: 0, + started_at: 0.0, + owner_client_id: None, + elf_path: None, + serial_handle: None, + reader_handle: None, + stop_flag: Arc::new(std::sync::atomic::AtomicBool::new(false)), + }, + ); + + let result = mgr.attach_reader(port, client); + assert!( + result.is_none(), + "attach_reader must return None when broadcaster is absent" + ); + + let leaked = mgr + .sessions + .get(port) + .map(|s| s.reader_client_ids.contains(client)) + .unwrap_or(false); + assert!( + !leaked, + "attach_reader must not mutate reader_client_ids when it \ + returns None — regression of FastLED/fbuild#51 where the \ + leaked id kept has_clients() true forever" + ); +} + +/// Regression guard for FastLED/fbuild#531: after a timeout/halt monitor +/// session ends, the HTTP `monitor` handler detaches its reader and then +/// closes the port when no clients remain, releasing the OS serial handle +/// so a follow-up pyserial/esptool open of the same port succeeds without +/// `fbuild daemon stop`. This locks in the manager contract that the +/// handler relies on: detach drops the last client, and close removes the +/// session (and its serial handle) from the manager entirely. +#[tokio::test] +async fn detach_then_close_releases_port_for_lone_monitor() { + let mgr = SharedSerialManager::new(); + let port = "COM_TEST_531"; + let client = "monitor-client"; + + // Simulate an open, single-reader monitor session (the timeout path): + // a broadcaster is present and one reader is attached. + let (tx, _rx) = broadcast::channel(BROADCAST_CHANNEL_SIZE); + mgr.broadcasters.insert(port.to_string(), tx); + let mut readers = std::collections::HashSet::new(); + readers.insert(client.to_string()); + mgr.sessions.insert( + port.to_string(), + super::SerialSession { + port: port.to_string(), + baud_rate: 115200, + is_open: true, + writer_client_id: None, + reader_client_ids: readers, + output_buffer: Default::default(), + total_bytes_read: 0, + total_bytes_written: 0, + started_at: 0.0, + owner_client_id: Some(client.to_string()), + elf_path: None, + serial_handle: None, + reader_handle: None, + stop_flag: Arc::new(std::sync::atomic::AtomicBool::new(false)), + }, + ); + + assert!( + mgr.has_clients(port), + "precondition: the lone monitor reader keeps the port busy" + ); + + // Mirror the handler cleanup sequence. + mgr.detach_reader(port, client); + assert!( + !mgr.has_clients(port), + "after the lone monitor detaches, no clients should remain" + ); + mgr.close_port(port, client).await.expect("close_port"); + + // The session (and its serial handle) must be gone — the OS port is + // released, so a non-fbuild client can reopen it. + assert!( + mgr.sessions.get(port).is_none(), + "close_port must remove the session so the OS handle is released \ + (regression of FastLED/fbuild#531)" + ); + assert!( + mgr.broadcasters.get(port).is_none(), + "close_port must also drop the broadcaster for the released port" + ); +} + +#[tokio::test] +async fn grace_close_removes_idle_port_after_delay() { + let mgr = Arc::new(SharedSerialManager::new()); + let port = "COM_TEST_GRACE_CLOSE"; + let client = "monitor-client"; + + mgr.sessions.insert( + port.to_string(), + super::SerialSession { + port: port.to_string(), + baud_rate: 115200, + is_open: true, + writer_client_id: None, + reader_client_ids: Default::default(), + output_buffer: Default::default(), + total_bytes_read: 0, + total_bytes_written: 0, + started_at: 0.0, + owner_client_id: Some(client.to_string()), + elf_path: None, + serial_handle: None, + reader_handle: None, + stop_flag: Arc::new(std::sync::atomic::AtomicBool::new(false)), + }, + ); + + assert!(mgr.close_port_after_grace_if_idle(port, client, Duration::from_millis(10))); + tokio::time::sleep(Duration::from_millis(50)).await; + + assert!( + mgr.sessions.get(port).is_none(), + "idle grace close should physically release the port after delay" + ); +} + +#[tokio::test] +async fn grace_close_is_canceled_by_new_reader() { + let mgr = Arc::new(SharedSerialManager::new()); + let port = "COM_TEST_GRACE_CANCEL"; + let client = "monitor-client"; + let next_client = "next-client"; + + let (tx, _rx) = broadcast::channel(BROADCAST_CHANNEL_SIZE); + mgr.broadcasters.insert(port.to_string(), tx); + mgr.sessions.insert( + port.to_string(), + super::SerialSession { + port: port.to_string(), + baud_rate: 115200, + is_open: true, + writer_client_id: None, + reader_client_ids: Default::default(), + output_buffer: Default::default(), + total_bytes_read: 0, + total_bytes_written: 0, + started_at: 0.0, + owner_client_id: Some(client.to_string()), + elf_path: None, + serial_handle: None, + reader_handle: None, + stop_flag: Arc::new(std::sync::atomic::AtomicBool::new(false)), + }, + ); + + assert!(mgr.close_port_after_grace_if_idle(port, client, Duration::from_millis(25))); + let rx = mgr.attach_reader(port, next_client); + assert!( + rx.is_some(), + "new reader should attach during pending close grace window" + ); + + tokio::time::sleep(Duration::from_millis(75)).await; + + assert!( + mgr.sessions.get(port).is_some(), + "new reader activity should cancel the pending physical close" + ); + assert!(mgr.has_clients(port)); +} + +#[test] +fn notify_port_renumbered_broadcasts_events_to_old_port() { + let mgr = SharedSerialManager::new(); + let old_port = "COM21"; + let new_port = "COM20"; + let (tx, mut rx) = broadcast::channel(BROADCAST_CHANNEL_SIZE); + mgr.broadcasters.insert(old_port.to_string(), tx); + + assert!(mgr.notify_port_renumbered( + old_port, + new_port, + "tracked_serial_move", + Some("15821020".to_string()) + )); + + assert_eq!( + rx.try_recv().unwrap(), + SerialStreamEvent::PortRenumbered { + port: old_port.to_string(), + new_port: new_port.to_string(), + reason: "tracked_serial_move".to_string(), + serial: Some("15821020".to_string()), + } + ); + assert_eq!( + rx.try_recv().unwrap(), + SerialStreamEvent::PortReattached { + port: new_port.to_string(), + previous_port: old_port.to_string(), + } + ); +} + +#[tokio::test] +async fn rebind_preserves_session_and_routes_writes_to_new_handle() { + let mgr = SharedSerialManager::new(); + let old_port = "COM21"; + let new_port = "COM20"; + let writer = "writer-client"; + let reader = "reader-client"; + let (tx, mut rx) = broadcast::channel(BROADCAST_CHANNEL_SIZE); + mgr.broadcasters.insert(old_port.to_string(), tx); + mgr.output_buffers.insert( + old_port.to_string(), + Arc::new(PortOutputBuffer { + buffer: std::sync::Mutex::new(VecDeque::with_capacity(OUTPUT_BUFFER_CAP)), + total_bytes_read: std::sync::atomic::AtomicU64::new(0), + }), + ); + let (old_fake, _old_writes) = FakeSerialPort::new(old_port); + let mut readers = std::collections::HashSet::new(); + readers.insert(reader.to_string()); + mgr.sessions.insert( + old_port.to_string(), + SerialSession { + port: old_port.to_string(), + baud_rate: 115200, + is_open: true, + writer_client_id: Some(writer.to_string()), + reader_client_ids: readers, + output_buffer: Default::default(), + total_bytes_read: 0, + total_bytes_written: 0, + started_at: 0.0, + owner_client_id: Some(writer.to_string()), + elf_path: None, + serial_handle: Some(Arc::new(Mutex::new(Box::new(old_fake)))), + reader_handle: None, + stop_flag: Arc::new(AtomicBool::new(false)), + }, + ); + let (new_fake, new_writes) = FakeSerialPort::new(new_port); + + assert!(mgr + .rebind_port_session_to_handle( + old_port, + new_port, + Arc::new(Mutex::new(Box::new(new_fake))), + "tracked_serial_move", + Some("15821020".to_string()), + ) + .await + .unwrap()); + + let session = mgr.sessions.get(old_port).expect("logical session remains"); + assert_eq!(session.port, new_port); + assert_eq!(session.writer_client_id.as_deref(), Some(writer)); + assert!(session.reader_client_ids.contains(reader)); + drop(session); + assert_eq!(mgr.reader_count(new_port), 1); + assert!(mgr.has_clients(new_port)); + + mgr.write_to_port(old_port, b"old-logical", writer) + .await + .unwrap(); + mgr.write_to_port(new_port, b"new-alias", writer) + .await + .unwrap(); + assert_eq!(&*new_writes.lock().unwrap(), b"old-logicalnew-alias"); + + assert_eq!( + rx.try_recv().unwrap(), + SerialStreamEvent::PortRenumbered { + port: old_port.to_string(), + new_port: new_port.to_string(), + reason: "tracked_serial_move".to_string(), + serial: Some("15821020".to_string()), + } + ); + assert_eq!( + rx.try_recv().unwrap(), + SerialStreamEvent::PortReattached { + port: new_port.to_string(), + previous_port: old_port.to_string(), + } + ); + + mgr.close_port(new_port, "test").await.unwrap(); + assert!(mgr.sessions.get(old_port).is_none()); + assert!(mgr.port_aliases.get(new_port).is_none()); +}