Skip to content

Conversation

@wjones127
Copy link
Contributor

Summary

  • Replace assert_string_matches() with new assert_str_matches! macro
  • Line-by-line ... wildcard matching (matches any content within a line)
  • Colored diff output on failure (red for extra, green for missing, dim for wildcard captures)
  • File/line/column info in panic messages

Test plan

  • Added unit tests for the macro and internal matching functions
  • Verified existing tests using the macro still pass

🤖 Generated with Claude Code

… diff

Replace `assert_string_matches()` with a new `assert_str_matches!` macro that
provides:
- Line-by-line `...` wildcard matching (matches any content within a line)
- Colored diff output on failure (red for extra, green for missing, dim for
  wildcard captures)
- File/line/column info in panic messages

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@github-actions github-actions bot added the enhancement New feature or request label Jan 2, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2026

Code Review - P0: Line alignment algorithm may produce confusing output for substitution cases. P1: assert_plan_node_equals signature changed from Result to unit type. P1: trim_whitespace appears unused after refactor.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2026

Full details on the issues above:

P0 - Line alignment (test.rs:553-588): The greedy lookahead in compare_strings can misalign substitutions. For actual=[A,X,B,C] vs expected=[A,Y,B,C], X and Y would be marked extra/missing in potentially confusing order. Add a test case to document expected behavior.

P1 - API change (test.rs:621-627): assert_plan_node_equals changed from returning lance_core::Result to (). This forces panics instead of allowing error propagation with ?. Check for external callers.

P1 - Dead code: trim_whitespace appears unused after refactor since compare_strings trims inline. Remove if unneeded.

Good: yansi is test-only, good test coverage for macro, correct cfg(test) guards.

@codecov
Copy link

codecov bot commented Jan 2, 2026

Codecov Report

❌ Patch coverage is 86.72199% with 32 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance/src/utils/test.rs 84.31% 29 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant