Update assert_order to support patterns on the same line#405
Update assert_order to support patterns on the same line#405
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/claude-code/test-helpers.sh`:
- Around line 117-131: The code compares numeric columns col_a and col_b but
doesn't handle when grep -bo fails and returns empty (causing "integer
expression expected") and also triggers ShellCheck SC2155 by combining
declaration+assignment; fix by declaring locals first (local the_line col_a
col_b) then assign them separately (the_line=$(echo ... | sed -n "${line_a}p");
col_a=$(echo "$the_line" | grep -bo "$pattern_a" | head -1 | cut -d: -f1) etc.),
validate each col (if [ -z "$col_a" ] || [ -z "$col_b" ] ) then print a clear
[FAIL] $test_name message showing the_line and indicate which pattern was not
found (include patterns $pattern_a/$pattern_b and the_line or full output) and
return 1; only if both are non-empty perform the numeric comparison using -lt.
🧹 Nitpick comments (1)
tests/claude-code/test-helpers.sh (1)
120-121: Consider edge case: overlapping or identical patterns.If
pattern_ais a substring ofpattern_b(e.g.,"test"vs"testing"),grep -bowill report the same starting column for both when the longer pattern appears first. This would cause the assertion to fail even though both patterns technically exist. Worth documenting this limitation or deciding if it needs handling.
| elif [ "$line_a" -eq "$line_b" ]; then | ||
| # Same line - check column positions | ||
| local the_line=$(echo "$output" | sed -n "${line_a}p") | ||
| local col_a=$(echo "$the_line" | grep -bo "$pattern_a" | head -1 | cut -d: -f1) | ||
| local col_b=$(echo "$the_line" | grep -bo "$pattern_b" | head -1 | cut -d: -f1) | ||
|
|
||
| if [ "$col_a" -lt "$col_b" ]; then | ||
| echo " [PASS] $test_name (both at line $line_a, A at col $col_a, B at col $col_b)" | ||
| return 0 | ||
| else | ||
| echo " [FAIL] $test_name" | ||
| echo " Expected '$pattern_a' before '$pattern_b' on line $line_a" | ||
| echo " But found A at col $col_a, B at col $col_b" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
Missing validation for empty column values will cause arithmetic error.
If grep -bo fails to find the pattern (possible with certain regex metacharacters that behave differently between grep -n and grep -bo), col_a or col_b will be empty, causing the arithmetic comparison on line 123 to fail with a bash error like integer expression expected.
Additionally, as flagged by ShellCheck (SC2155), declaring and assigning in one statement masks the return value, making it harder to detect failures.
🛡️ Proposed fix with validation and separated declarations
elif [ "$line_a" -eq "$line_b" ]; then
# Same line - check column positions
- local the_line=$(echo "$output" | sed -n "${line_a}p")
- local col_a=$(echo "$the_line" | grep -bo "$pattern_a" | head -1 | cut -d: -f1)
- local col_b=$(echo "$the_line" | grep -bo "$pattern_b" | head -1 | cut -d: -f1)
+ local the_line
+ the_line=$(echo "$output" | sed -n "${line_a}p")
+ local col_a
+ col_a=$(echo "$the_line" | grep -bo "$pattern_a" | head -1 | cut -d: -f1)
+ local col_b
+ col_b=$(echo "$the_line" | grep -bo "$pattern_b" | head -1 | cut -d: -f1)
+
+ if [ -z "$col_a" ] || [ -z "$col_b" ]; then
+ echo " [FAIL] $test_name: could not determine column positions on line $line_a"
+ return 1
+ fi
if [ "$col_a" -lt "$col_b" ]; then🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 119-119: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 120-120: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 121-121: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In `@tests/claude-code/test-helpers.sh` around lines 117 - 131, The code compares
numeric columns col_a and col_b but doesn't handle when grep -bo fails and
returns empty (causing "integer expression expected") and also triggers
ShellCheck SC2155 by combining declaration+assignment; fix by declaring locals
first (local the_line col_a col_b) then assign them separately (the_line=$(echo
... | sed -n "${line_a}p"); col_a=$(echo "$the_line" | grep -bo "$pattern_a" |
head -1 | cut -d: -f1) etc.), validate each col (if [ -z "$col_a" ] || [ -z
"$col_b" ] ) then print a clear [FAIL] $test_name message showing the_line and
indicate which pattern was not found (include patterns $pattern_a/$pattern_b and
the_line or full output) and return 1; only if both are non-empty perform the
numeric comparison using -lt.
obra
added
claude-code
2 participants
Update assert_order in test-helpers to support checking patterns order on the same line
Motivation and Context
assert_order in test-helpers currently checks for patterns on different lines. If the patterns are in the correct order on the same line, it will still fail. This fix is to check the column order if they are on the same line.
How Has This Been Tested?
Tested by tests/claude-code/test-subagent-driven-development.sh
Breaking Changes
No
Types of changes
Checklist
Additional context
Summary by CodeRabbit