Skip to content

Trim test names so CATCH2 does not ignore them#1057

Merged
elazarg merged 2 commits intomainfrom
long-test-names
Mar 15, 2026
Merged

Trim test names so CATCH2 does not ignore them#1057
elazarg merged 2 commits intomainfrom
long-test-names

Conversation

@elazarg
Copy link
Collaborator

@elazarg elazarg commented Mar 15, 2026

Summary by CodeRabbit

  • Style

    • Code formatting consolidation and alignment adjustments across internal modules to improve consistency.
  • Tests

    • Enhanced test infrastructure with improved naming and organizational utilities for better test management.
  • Chores

    • Refactored internal test utilities and helper functions for maintainability.

elazarg added 2 commits March 15, 2026 04:22
Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 15, 2026

Walkthrough

This PR includes formatting consolidations in IR handling code and introduces a compile-time test name bounding mechanism with a new BoundedTestName struct that enforces maximum test name length while preserving uniqueness via hash suffixes, then migrates existing test case macros to use the new bounded-name variants.

Changes

Cohort / File(s) Summary
IR formatting consolidations
src/ir/assertions.cpp, src/ir/unmarshal.cpp
Compress multi-line case statements to single-line format in AssertExtractor, and adjust function signature and error message line breaks in unmarshal without altering semantics.
Test infrastructure updates
src/test/test_verify.hpp
Introduce BoundedTestName struct with constexpr constructor, add BOUNDED_TEST_CASE macro wrapper, migrate all test case macros to use bounded names with hash suffixes, remove inline from hash_combine, and eliminate obsolete includes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main change: trimming test names to prevent CATCH2 from ignoring them, which aligns with the core functionality added in test_verify.hpp.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch long-test-names
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can customize the high-level summary generated by CodeRabbit.

Configure the reviews.high_level_summary_instructions setting to provide custom instructions for generating the high-level summary.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/test/test_verify.hpp`:
- Around line 113-149: Add deterministic unit tests for BoundedTestName that
validate its invariants: (1) passthrough behavior for short names (construct
constexpr BoundedTestName with a name length <= BoundedTestName::MAX_LEN and
assert data contains the full input and is null-terminated); (2) truncation
behavior for long names (construct with length > MAX_LEN and assert resulting
data length is exactly MAX_LEN, contains the prefix of length MAX_LEN-6 followed
by the literal ".." and a 4-hex-digit suffix); and (3) suffix
determinism/variance (construct two different long inputs that differ only
beyond the prefix and assert their 4-hex suffixes differ, and construct the same
long input twice and assert the suffixes are identical). Use the BoundedTestName
constructor and its data member in constexpr contexts where possible and add
focused test cases that fail deterministically when any invariant is violated.
- Around line 134-146: The truncation currently appends a 4-hex suffix (16 bits)
using prefix_len = MAX_LEN - 6 and four assignments from hash; increase the
suffix to 8 hex digits (32 bits) to improve collision resistance by making the
suffix two dots plus eight hex chars (i.e., suffix length 10) so set prefix_len
= MAX_LEN - 10, then write eight hex characters into data[prefix_len + 2]
through data[prefix_len + 9] using the hex[] lookup with appropriate bit-shifts
(e.g., shifts for 28,24,20,16,12,8,4,0) against the hash variable, and keep the
null terminator at data[MAX_LEN]; update any related comments and ensure hash is
wide enough (32-bit) where needed.
- Line 129: Add a direct include of the fixed-width integer header so the use of
uint32_t in test_verify.hpp is not relying on transitive includes: insert
`#include` <cstdint> near the other includes at the top of
src/test/test_verify.hpp so the declaration "uint32_t hash = 2166136261u;" is
valid and portable.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: dd4a628c-f099-4ea0-a0a5-acd2fd64d240

📥 Commits

Reviewing files that changed from the base of the PR and between 4790e81 and 74e02f4.

📒 Files selected for processing (4)
  • scripts/format-code
  • src/ir/assertions.cpp
  • src/ir/unmarshal.cpp
  • src/test/test_verify.hpp

@elazarg elazarg merged commit e51569a into main Mar 15, 2026
16 checks passed
@elazarg elazarg deleted the long-test-names branch March 15, 2026 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant