test: cover default signoff regex for bot and regular names - #463
Conversation
The existing SignoffValidator tests pass an inline regex, so the default pattern in rules_catalog was never exercised. Build the rule via RuleBuilder and assert the shipped default: - accepts a bracketed bot name (Signed-off-by: dependabot[bot] <...>) - accepts a regular Name <email> signoff - rejects a message with no signoff trailer
✅ Deploy Preview for commit-check ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughTest module ChangesSignoff Validator Default Rule Tests
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/engine_test.py (1)
651-662: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an assertion-friendly fallback for
next()
next(r for r in rules if r.check == "require_signed_off_by")can fail with a bareStopIteration, which makes this test harder to diagnose. Use a default plus anassertso the failure message is explicit.🤖 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 `@tests/engine_test.py` around lines 651 - 662, The helper _default_signoff_rule currently uses next() to find the require_signed_off_by rule, which can raise a bare StopIteration and make the test failure unclear. Update this logic to use an assertion-friendly fallback in the generator lookup, then assert the result is not None before returning it, so failures in RuleBuilder.build_all_rules() are explicit and easier to diagnose.
🤖 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 `@tests/engine_test.py`:
- Around line 651-662: The helper _default_signoff_rule currently uses next() to
find the require_signed_off_by rule, which can raise a bare StopIteration and
make the test failure unclear. Update this logic to use an assertion-friendly
fallback in the generator lookup, then assert the result is not None before
returning it, so failures in RuleBuilder.build_all_rules() are explicit and
easier to diagnose.
Merging this PR will improve performance by 36.22%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_empty_message_passes |
1,016.5 µs | 746.2 µs | +36.22% |
| 🆕 | test_default_signoff_accepts_bot_name |
N/A | 255.8 µs | N/A |
| 🆕 | test_default_signoff_accepts_regular_name |
N/A | 255.4 µs | N/A |
| 🆕 | test_default_signoff_rejects_missing_signoff |
N/A | 924.3 µs | N/A |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing test/signoff-default-regex-coverage (823c06c) with main (092a824)
Footnotes
-
109 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
The existing SignoffValidator tests pass an inline regex, so the default pattern in rules_catalog was never exercised. Build the rule via RuleBuilder and assert the shipped default:
related to #462
Summary by CodeRabbit