mruby-regexp: split the test file by subject - #7088
Merged
Merged
Conversation
test/regexp.rb had grown to 3338 lines holding every subject at once, three times the size of the largest test file elsewhere in the tree. The other gems split by tested class (mruby-io keeps io.rb, file.rb and file_test.rb; mruby-task keeps task.rb and queue.rb); this suite is too large for that alone, so it splits one level finer, by subject: - regexp.rb: the class API. Construction, `match`/`match?`/`=~`/`===`, Symbol subjects, and the object protocol (`escape`, `inspect`, `to_s`, `==`, `hash`, `options`, `casefold?`). - regexp_syntax.rb: what patterns mean. Classes, quantifiers, alternation, anchors, groups, backreferences, named captures, lookaround, inline options, /i and /x. - regexp_utf8.rb: characters against bytes. Multibyte atoms, `\u` escapes, invalid and overlong sequences, byte classes, binary subjects. - match_data.rb: `MatchData` and the match globals. - string_regexp.rb: the String methods this gem defines. `match`, `=~`, `sub`, `gsub`, `scan`, `split`, and their dispatch checks with the helper classes they share. - string_index.rb: the core String methods this gem extends with a regexp form. `[]`, `[]=`, `slice!`, the index family, `partition`, `start_with?`, and the override surface. A pure move: every block is carried verbatim in its original order, and no block changes meaning by moving. "String#split and String#scan see the empty iteration's capture" stays with the empty-iteration quartet in regexp_syntax.rb because it argues about the engine, not about String. `spec.test_rbfiles` globs the test directory, so the build picks the new files up as it stands; the conditional subtraction in mrbgem.rake names only the two case-folding files and is unaffected. Its comment now points at the unconditional files as a group rather than at test/regexp.rb by name.
📝 WalkthroughWalkthroughThe pull request adds extensive regression tests for mruby regexp syntax, UTF-8 behavior, MatchData, match globals, and regexp-aware String operations. ChangesRegexp behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 (1)
mrbgems/mruby-regexp/test/regexp_utf8.rb (1)
468-474: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBuild the pattern with
<<instead of+=.The loop runs about 32,769 times. Each
s += ...allocates a new String and copies the whole accumulated buffer, so the loop copies on the order of a gigabyte to build a ~90 KB pattern.String#<<appends in place and makes the loop linear. The resulting pattern is identical.♻️ Proposed change
s = "[" i = 0x80 while i <= 0x8080 - s += utf8.call(i) + s << utf8.call(i) i += 1 end - s += "]" + s << "]"🤖 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 `@mrbgems/mruby-regexp/test/regexp_utf8.rb` around lines 468 - 474, Update the pattern-building loop in the UTF-8 regexp test to append each utf8.call(i) result in place with String#<< instead of allocating through +=; preserve the existing loop bounds and resulting pattern contents.
🤖 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 `@mrbgems/mruby-regexp/test/regexp_utf8.rb`:
- Around line 468-474: Update the pattern-building loop in the UTF-8 regexp test
to append each utf8.call(i) result in place with String#<< instead of allocating
through +=; preserve the existing loop bounds and resulting pattern contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a3c14d4-647e-43da-a77b-360246358f80
📒 Files selected for processing (7)
mrbgems/mruby-regexp/mrbgem.rakemrbgems/mruby-regexp/test/match_data.rbmrbgems/mruby-regexp/test/regexp.rbmrbgems/mruby-regexp/test/regexp_syntax.rbmrbgems/mruby-regexp/test/regexp_utf8.rbmrbgems/mruby-regexp/test/string_index.rbmrbgems/mruby-regexp/test/string_regexp.rb
Contributor
Author
|
The "pure move" claim above is checkable mechanically. Save the script below next to the repository and run it on this branch: It concatenates the touched
Together these say every case is carried verbatim and each new file keeps the relative order the original had. They say nothing about which file a case landed in; that is the judgement the PR body argues for. The script fails on the three mutations I seeded to check it: an edited title (caught by 1), two blocks swapped (3), and two lines swapped inside one block (2). #!/bin/sh
# Check that this PR only moves test code around.
#
# sh verify-split.sh [BASE] [HEAD] # defaults: merge-base with origin/master, and HEAD
set -eu
dir=mrbgems/mruby-regexp/test
base=${1:-$(git merge-base HEAD origin/master)}
head=${2:-HEAD}
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
# Every .rb under the test dir that the diff touches, on either side.
git diff --name-only "$base" "$head" -- "$dir/*.rb" >"$tmp/paths"
cat_rev() {
: >"$2"
while IFS= read -r p; do
if git cat-file -e "$1:$p" 2>/dev/null; then git show "$1:$p" >>"$2"; fi
done <"$tmp/paths"
}
cat_rev "$base" "$tmp/before.rb"
cat_rev "$head" "$tmp/after.rb"
lines() { grep -v '^[[:space:]]*$' "$1" | LC_ALL=C sort; }
titles() { grep '^assert' "$1" || true; }
# One line per assert block, blank lines dropped, so blocks compare as units.
blocks() {
awk '/^assert\(/ { if (n) print b; b = $0; n = 1; next }
n && $0 !~ /^[[:space:]]*$/ { b = b " | " $0 }
END { if (n) print b }' "$1" | LC_ALL=C sort
}
check() {
"$1" "$tmp/before.rb" >"$tmp/b.$1"
"$1" "$tmp/after.rb" >"$tmp/a.$1"
if diff -u "$tmp/b.$1" "$tmp/a.$1" >"$tmp/d.$1"; then
echo "ok $1: $(wc -l <"$tmp/b.$1") identical"
else
echo "FAIL $1 differ:"; sed -n '3,30p' "$tmp/d.$1" | cut -c1-100; exit 1
fi
}
check lines # nothing added, dropped or edited, anywhere in these files
check blocks # every assert block carried verbatim, as a unit
# Each destination file keeps its cases in the order the original had them.
titles "$tmp/before.rb" >"$tmp/before.titles"
while IFS= read -r p; do
git cat-file -e "$head:$p" 2>/dev/null || continue
git show "$head:$p" >"$tmp/f.rb"
titles "$tmp/f.rb" >"$tmp/f.titles"
awk -v f="$p" '
NR == FNR { o[++n] = $0; next }
{ m++
while (i < n && o[i+1] != $0) i++
if (i >= n) { print " out of order: " $0; bad = 1; exit 1 }
i++ }
END { if (!bad) printf "ok %s: %d cases in original order\n", f, m }
' "$tmp/before.titles" "$tmp/f.titles" || { echo "FAIL $p reorders cases"; exit 1; }
done <"$tmp/paths"
echo "PASS: a pure move" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test/regexp.rb had grown to 3338 lines and 219 cases holding every subject at
once, three times the size of the largest test file elsewhere in the tree
(mruby-string-ext's string.rb, at 974 lines). Finding the cases about one
subject meant scanning past all the others.
The tree splits test files by tested class: mruby-io keeps io.rb, file.rb and
file_test.rb, mruby-task keeps task.rb and queue.rb, and mruby-string-ext
splits by the class its methods land on. This suite is too large for that cut
alone, since the String integration is half of it by itself, so it splits one
level finer, by subject:
test/regexp.rb(264 lines): the class API. Construction,match/match?/=~/===, Symbol subjects, and the object protocol(
escape,inspect,to_s,==,hash,options,casefold?).test/regexp_syntax.rb(876): what patterns mean. Classes, quantifiers,alternation, anchors, groups, backreferences, named captures, lookaround,
inline options,
/iand/x.test/regexp_utf8.rb(481): characters against bytes. Multibyte atoms,\uescapes, invalid and overlong sequences, byte classes, binary subjects.test/match_data.rb(219):MatchDataand the match globals.test/string_regexp.rb(776): the String methods this gem defines:match,=~,sub,gsub,scan,split, and their dispatch checkswith the helper classes they share.
test/string_index.rb(717): the core String methods this gem extends witha regexp form:
[],[]=,slice!, the index family,partition,start_with?, and the override surface.The three build-conditional files (ascii_case.rb, unicode_case.rb,
symbol_regexp.rb) are untouched.
A pure move
Every block is carried verbatim in its original order, and no block changes
meaning by moving: the multiset of non-blank lines is identical before and
after the split. One title reads as String but stays put. "String#split and
String#scan see the empty iteration's capture" belongs to the empty-iteration
quartet in regexp_syntax.rb, which argues about the engine, not about String.
spec.test_rbfilesglobs the test directory, so the build picks the newfiles up as it stands. The conditional subtraction in mrbgem.rake names only
the two case-folding files and is unaffected; its comment now points at the
unconditional files as a group rather than at test/regexp.rb by name.
rake testis clean: 2049 cases, 0 failures, and the same skips as beforethe split.
Summary by CodeRabbit