Improve test coverage related to 'bracket-terminal/consoles'#104
Conversation
📝 WalkthroughWalkthroughThis PR adds comprehensive unit test infrastructure and test coverage to bracket-terminal's console system. It introduces ChangesTest Infrastructure and Utilities
Console Implementation Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
bracket-terminal/src/consoles/flexible_console.rs (2)
731-731: ⚡ Quick winSimplify unnecessarily complex expression.
The expression
"".len() as i32evaluates to0. Replace it with the literal0for clarity.♻️ Proposed fix
- console.print("".len() as i32, 0, "AB"); + console.print(0, 0, "AB");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/consoles/flexible_console.rs` at line 731, The call to console.print uses the unnecessary expression "\"\".len() as i32" which always equals 0; replace that expression with the literal 0 to improve clarity and performance by changing console.print("".len() as i32, 0, "AB") to console.print(0, 0, "AB") in the code path that invokes the print method (the console.print call in flexible_console.rs).
496-502: ⚡ Quick winMisleading test name.
The test name
flexi_console_at_mapping_differs_from_test_console_row_major_mappingsuggests a comparison with aTestConsoleinstance, but the test only verifiesFlexiConsole's bottom-origin mapping behavior. Consider renaming to better reflect what's actually tested, e.g.,at_uses_bottom_origin_unlike_row_major_mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/consoles/flexible_console.rs` around lines 496 - 502, Rename the test function flexi_console_at_mapping_differs_from_test_console_row_major_mapping to a name that reflects it only verifies FlexiConsole's bottom-origin mapping (suggested: at_uses_bottom_origin_unlike_row_major_mapping); update the test declaration (the function name) and any references to it, keeping the body unchanged (uses FlexiConsole::init and calls flexi.at(0, 0) and flexi.at(0, 3)).bracket-terminal/src/test_utils.rs (1)
32-34: 💤 Low valueConsider adding debug assertions for coordinate bounds.
The
at()method performs no bounds validation before calculating the index. While theConsoletrait doesn't requireat()to check bounds (that's the role ofin_boundsandtry_at), adding a debug assertion would help catch test bugs earlier and make debugging easier.🛡️ Optional: Add debug assertions
fn at(&self, x: i32, y: i32) -> usize { + debug_assert!( + x >= 0 && x < self.size.0 as i32 && y >= 0 && y < self.size.1 as i32, + "at() called with out-of-bounds coordinates: ({}, {}) for console size {:?}", + x, y, self.size + ); y as usize * self.size.0 as usize + x as usize }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/test_utils.rs` around lines 32 - 34, Add a debug assertion to the at(&self, x: i32, y: i32) -> usize method to validate coordinates before computing the index; e.g., use debug_assert!(x >= 0 && y >= 0 && x < self.size.0 && y < self.size.1) or call an existing in_bounds(x,y) helper to assert bounds on self.size, then proceed to compute the index as before—this will catch test bugs without changing release behavior.bracket-terminal/src/consoles/sprite_console.rs (1)
324-330: 💤 Low valueRemove or enhance this redundant test.
This test is redundant with the parametrized
at_uses_bottom_origin_storagetest above, which already validates all the same coordinate mappings. The test name suggests it demonstrates how the mapping "differs from row-major," but it doesn't show what row-major mapping would produce or make any comparison.Either remove this test or enhance it to actually demonstrate the difference (e.g., by showing what row-major
y * width + xwould produce vs. the bottom-origin formula).♻️ Suggested fix: remove the redundant test
- #[test] - fn sprite_console_at_mapping_differs_from_test_console_row_major_mapping() { - let sprite_console = SpriteConsole::init(10, 4, 0); - - assert_eq!(sprite_console.at(0, 0), 30); - assert_eq!(sprite_console.at(0, 3), 0); - } -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/consoles/sprite_console.rs` around lines 324 - 330, The test function sprite_console_at_mapping_differs_from_test_console_row_major_mapping is redundant with the parametrized at_uses_bottom_origin_storage test; either delete this test entirely, or modify it to explicitly compare bottom-origin mapping to row-major by calling SpriteConsole::init(...) and using its at(x,y) results vs the row-major formula (y * width + x) so the test asserts the differing values; locate the test by its function name and the SpriteConsole::init and at methods to update or remove accordingly.bracket-terminal/src/consoles/sparse_console.rs (2)
460-466: 💤 Low valueConsider removing this redundant test.
This test duplicates coverage from the parameterized
at_uses_bottom_origin_storagetest above (lines 447-458), which already verifiesat(0, 0) == 30andat(9, 3) == 9among other cases. The test name suggests a comparison with TestConsole, but no such comparison is present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/consoles/sparse_console.rs` around lines 460 - 466, Remove the redundant unit test sparse_console_at_mapping_differs_from_test_console_row_major_mapping (or replace it with a real comparison) because its assertions are already covered by the parameterized test at_uses_bottom_origin_storage; locate the test function named sparse_console_at_mapping_differs_from_test_console_row_major_mapping and delete its whole #[test] fn block (or alternatively rewrite it to actually compare SparseConsole::at mapping against TestConsole::at if a distinct comparison is required).
543-585: 💤 Low valueConsider verifying dirty flag behavior in set_bg tests.
The tests correctly validate tile creation and modification, but don't assert that
is_dirtyis set whenset_bgmodifies state (lines 544-574). While the implementation does set the dirty flag (line 153), explicit verification would improve test completeness.Example assertion to add
fn set_bg_updates_existing_tile_backgrounds_with_same_idx() { let mut console = SparseConsole::init(10, 4); let old_bg = rgba(1, 1, 1, 255); let new_bg = rgba(9, 8, 7, 6); console.set(2, 1, rgba(1, 2, 3, 4), old_bg, 65); console.set(2, 1, rgba(5, 6, 7, 8), old_bg, 66); + console.clear_dirty(); console.set_bg(2, 1, new_bg); + assert!(console.is_dirty); assert_eq!(console.tiles.len(), 2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/consoles/sparse_console.rs` around lines 543 - 585, The tests for set_bg (functions set_bg_updates_existing_tile_backgrounds_with_same_idx, set_bg_creates_space_tile_when_no_existing_tile_matches, set_bg_ignores_out_of_bounds_coordinates) are missing assertions that the console dirty flag is set when state changes; update each test that expects a modification to assert console.is_dirty (or the appropriate dirty field) is true after calling SparseConsole::set_bg, and ensure the out-of-bounds test asserts the dirty flag remains false when no tiles were modified, referencing the SparseConsole::set_bg method and the is_dirty field to locate the relevant behavior to verify.bracket-terminal/src/consoles/virtual_console.rs (1)
707-720: ⚡ Quick winConsider adding explicit edge-case tests for the wrapping bug fix.
The test validates the overall wrapping behavior but doesn't explicitly document the specific edge cases mentioned in the PR description:
- Wrapping at exact width boundary (e.g., "abc" with width 3)
- Prevention of trailing empty lines when wrapping exhausts the buffer
While this test indirectly validates the fix (height is exactly 3, not 4+), an explicit test case would make the fix more self-documenting and prevent regression.
💡 Optional test to explicitly validate edge cases
#[test] fn from_text_wraps_at_exact_width_and_omits_trailing_empty_lines() { // Edge case 1: Line exactly at width boundary let console = VirtualConsole::from_text("abc", 3); assert_eq!(console.height, 1); assert_eq!(console.tiles[console.at(0, 0)].glyph, 97); assert_eq!(console.tiles[console.at(2, 0)].glyph, 99); // Edge case 2: Wrapping that exhausts buffer (no trailing empty line) let console = VirtualConsole::from_text("abcd", 3); assert_eq!(console.height, 2); // Should be ["abc", "d"], not ["abc", "d", ""] assert_eq!(console.tiles[console.at(0, 0)].glyph, 97); // "abc" on y=0 assert_eq!(console.tiles[console.at(0, 1)].glyph, 100); // "d" on y=1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/consoles/virtual_console.rs` around lines 707 - 720, Add explicit unit tests covering the two edge cases described: create new tests that call VirtualConsole::from_text to assert (1) a line exactly equal to the width (e.g., from_text("abc", 3)) yields height == 1 and correct glyphs at at(0,0) and at(2,0), and (2) wrapping that fills a line and leaves a partial second line without producing a trailing empty line (e.g., from_text("abcd", 3)) yields height == 2 and glyphs at at(0,0) == 'a' and at(0,1) == 'd'; add these tests alongside the existing from_text_wraps_lines_and_prints_content to make the wrapping bug coverage explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@bracket-terminal/src/consoles/flexible_console.rs`:
- Line 731: The call to console.print uses the unnecessary expression
"\"\".len() as i32" which always equals 0; replace that expression with the
literal 0 to improve clarity and performance by changing console.print("".len()
as i32, 0, "AB") to console.print(0, 0, "AB") in the code path that invokes the
print method (the console.print call in flexible_console.rs).
- Around line 496-502: Rename the test function
flexi_console_at_mapping_differs_from_test_console_row_major_mapping to a name
that reflects it only verifies FlexiConsole's bottom-origin mapping (suggested:
at_uses_bottom_origin_unlike_row_major_mapping); update the test declaration
(the function name) and any references to it, keeping the body unchanged (uses
FlexiConsole::init and calls flexi.at(0, 0) and flexi.at(0, 3)).
In `@bracket-terminal/src/consoles/sparse_console.rs`:
- Around line 460-466: Remove the redundant unit test
sparse_console_at_mapping_differs_from_test_console_row_major_mapping (or
replace it with a real comparison) because its assertions are already covered by
the parameterized test at_uses_bottom_origin_storage; locate the test function
named sparse_console_at_mapping_differs_from_test_console_row_major_mapping and
delete its whole #[test] fn block (or alternatively rewrite it to actually
compare SparseConsole::at mapping against TestConsole::at if a distinct
comparison is required).
- Around line 543-585: The tests for set_bg (functions
set_bg_updates_existing_tile_backgrounds_with_same_idx,
set_bg_creates_space_tile_when_no_existing_tile_matches,
set_bg_ignores_out_of_bounds_coordinates) are missing assertions that the
console dirty flag is set when state changes; update each test that expects a
modification to assert console.is_dirty (or the appropriate dirty field) is true
after calling SparseConsole::set_bg, and ensure the out-of-bounds test asserts
the dirty flag remains false when no tiles were modified, referencing the
SparseConsole::set_bg method and the is_dirty field to locate the relevant
behavior to verify.
In `@bracket-terminal/src/consoles/sprite_console.rs`:
- Around line 324-330: The test function
sprite_console_at_mapping_differs_from_test_console_row_major_mapping is
redundant with the parametrized at_uses_bottom_origin_storage test; either
delete this test entirely, or modify it to explicitly compare bottom-origin
mapping to row-major by calling SpriteConsole::init(...) and using its at(x,y)
results vs the row-major formula (y * width + x) so the test asserts the
differing values; locate the test by its function name and the
SpriteConsole::init and at methods to update or remove accordingly.
In `@bracket-terminal/src/consoles/virtual_console.rs`:
- Around line 707-720: Add explicit unit tests covering the two edge cases
described: create new tests that call VirtualConsole::from_text to assert (1) a
line exactly equal to the width (e.g., from_text("abc", 3)) yields height == 1
and correct glyphs at at(0,0) and at(2,0), and (2) wrapping that fills a line
and leaves a partial second line without producing a trailing empty line (e.g.,
from_text("abcd", 3)) yields height == 2 and glyphs at at(0,0) == 'a' and
at(0,1) == 'd'; add these tests alongside the existing
from_text_wraps_lines_and_prints_content to make the wrapping bug coverage
explicit.
In `@bracket-terminal/src/test_utils.rs`:
- Around line 32-34: Add a debug assertion to the at(&self, x: i32, y: i32) ->
usize method to validate coordinates before computing the index; e.g., use
debug_assert!(x >= 0 && y >= 0 && x < self.size.0 && y < self.size.1) or call an
existing in_bounds(x,y) helper to assert bounds on self.size, then proceed to
compute the index as before—this will catch test bugs without changing release
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d54402ad-6a23-4f4a-8719-b2f65903600b
📒 Files selected for processing (9)
bracket-terminal/Cargo.tomlbracket-terminal/src/consoles/console.rsbracket-terminal/src/consoles/flexible_console.rsbracket-terminal/src/consoles/simple_console.rsbracket-terminal/src/consoles/sparse_console.rsbracket-terminal/src/consoles/sprite_console.rsbracket-terminal/src/consoles/virtual_console.rsbracket-terminal/src/lib.rsbracket-terminal/src/test_utils.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: ✅ Check (ubuntu-latest)
- GitHub Check: ✅ Check (windows-latest)
- GitHub Check: ✅ Check (macos-latest)
- GitHub Check: 🕸️ WASM Build
- GitHub Check: 🧪 Test Suite
- GitHub Check: 🚨 Lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format all Rust code usingcargo fmt --all
Fix allcargo clippywarnings — the CI enforces-D warnings
Add tests for new functionality in the relevant module; for split domains, prefer colocatedtests.rs
Files:
bracket-terminal/src/lib.rsbracket-terminal/src/consoles/console.rsbracket-terminal/src/consoles/sparse_console.rsbracket-terminal/src/consoles/flexible_console.rsbracket-terminal/src/test_utils.rsbracket-terminal/src/consoles/virtual_console.rsbracket-terminal/src/consoles/simple_console.rsbracket-terminal/src/consoles/sprite_console.rs
🔇 Additional comments (39)
bracket-terminal/src/consoles/flexible_console.rs (10)
465-472: LGTM!
474-481: LGTM!
483-494: LGTM!
504-554: LGTM!
556-617: LGTM!
619-651: LGTM!
653-693: LGTM!
695-725: LGTM!
727-730: LGTM!Also applies to: 732-755
757-794: LGTM!bracket-terminal/src/lib.rs (1)
12-14: LGTM!bracket-terminal/src/test_utils.rs (1)
1-145: LGTM!bracket-terminal/src/consoles/console.rs (3)
7-8: LGTM!
31-31: LGTM!
214-308: LGTM!bracket-terminal/Cargo.toml (1)
55-56: Confirm dev-dependency versions are current and show no known advisories
mockall = "0.14.0"andrstest = "0.26.1"match crates.io’s latest stable versions, and the security advisories query returned no vulnerabilities for either package.bracket-terminal/src/consoles/sprite_console.rs (6)
287-298: LGTM!
300-309: LGTM!
311-322: LGTM!
332-372: LGTM!
374-438: LGTM!
440-528: LGTM!bracket-terminal/src/consoles/sparse_console.rs (6)
428-445: LGTM!
468-515: LGTM!
587-653: LGTM!
655-711: LGTM!
713-765: LGTM!
767-792: LGTM!bracket-terminal/src/consoles/simple_console.rs (7)
418-435: LGTM!
437-456: LGTM!
458-533: LGTM!
535-582: LGTM!
584-606: LGTM!
608-638: LGTM!
640-713: LGTM!bracket-terminal/src/consoles/virtual_console.rs (4)
46-90: LGTM!
721-784: LGTM!
449-449: rstest 0.26.1 is current (latest stable on crates.io) — the crate’s latest stable version is 0.26.1, so the dependency in the tests aligns with the current release.
446-784: Ensurecargo fmt+cargo clippygates pass for the new tests
cargo fmt --all -- --check/cargo clippy --workspace --all-targets -- -D warnings -A clippy::multiple_crate_versionscouldn’t be executed here becauserustupcan’t select a Cargo version (no default toolchain configured). Please run these in CI or locally to confirm the test code complies.
1 participant
What
Added comprehensive unit tests for console implementations and shared test utilities across the console subsystem.
Included test coverage for:
Also added reusable TestConsole support utilities for validating trait default implementations and console semantics consistently across implementations.
Additionally, fixed a bug in VirtualConsole::from_text() line wrapping logic.
Why
The console subsystem had very limited implementation-specific test coverage despite containing a large amount of coordinate translation, clipping, sparse storage, rendering, and text layout logic.
While adding tests for VirtualConsole, an issue was discovered in VirtualConsole::from_text().
The wrapping logic previously used:
which allowed a line to exceed the configured width before being flushed.
After correcting this to:
Another issue became visible: the implementation always pushed the remaining buffer after processing each input line, even when the buffer had already been flushed and was empty.
This produced unintended empty lines.
Example:
Input text:
Width:
Expected logical lines:
Previous behavior:
Because:
The implementation was updated to avoid appending empty trailing buffers after wrapping.
Checklist
Required
cargo check --allpassescargo fmt --all -- --checkpassescargo clippy --workspace --all-targets -- -D warnings -A clippy::multiple-crate-versionspassescargo test --allpassesCloses #26)Functional Validation
Configuration & Docs
README.md,ARCHITECTURE.md, or relevant manual pages, if applicable)If Applicable
cargo auditlocally if needed)Summary by CodeRabbit
Bug Fixes
from_textmethod to correctly handle line breaks and trailing content.Tests