test: build wide shifts from a variable so the file still compiles - #7117
Conversation
An integer literal that does not fit `mrb_int` is rejected while the file is compiled, not when the line runs, so `1 << 63` in a test drops the whole test file on a build with a 32-bit `mrb_int` and no bigint. Nothing reports it: the tests are not skipped and not failed, they are simply absent, and the suite goes green with fewer tests than it had. Three test files pay for this today. Counted on a build with `MRB_INT32` and no bigint: * `test/t/string.rb`, from `~(-1 << 63)`: 71 tests * `mrbgems/mruby-random/test/random.rb`, from `1 << 62`: 13 tests * `mrbgems/mruby-socket/test/addrinfo.rb`, from `1 << 40`: 10 tests All three already meant to handle a narrow `mrb_int`. The `rescue nil` in mruby-random and the `skip` in mruby-socket never got the chance to run, and the `begin`/`rescue` in `String#bytesplice` covers a line the compiler had already thrown away. Take the shift width from a variable in each of them. The shift is then a run time operation that raises `RangeError` where `mrb_int` is too narrow, which is what the surrounding `rescue` and `skip` were written for.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe tests now compute wide integer values at runtime. The Addrinfo tests also skip when 64-bit integer support is unavailable. Existing overflow and ChangesInteger portability tests
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@mrbgems/mruby-random/test/random.rb`:
- Around line 173-177: Update the wide-range test around the shift, capability
probe, and `rand(-hi..hi)` call so support detection does not depend on the
operation’s potentially overflowing result. Use an independent
representation/path check to skip only when wide ranges are unsupported, then
assert that the actual valid-range call returns an Integer instead of treating
nil as unsupported.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c50007ed-cad4-4d0a-9b47-67252235f195
📒 Files selected for processing (3)
mrbgems/mruby-random/test/random.rbmrbgems/mruby-socket/test/addrinfo.rbtest/t/string.rb
`Kernel#rand integer range overflow` reaches its wide bounds only where `mrb_int` is 64 bits wide, and it decided that by calling `rand` on the very range whose overflow it guards against. The regression it covers returns `nil` for that range, which the guard read as "this build has no integer range path", so every assertion behind it was skipped and the suite stayed green with the bug in place. Ask what the bounds are instead. `1 << 62` raises `RangeError` where `mrb_int` is 32 bits and mruby-bigint is absent, and where the gem promotes it the result is a big integer, which a core method taking an `mrb_int` rejects with the same error. Neither kind of bound reaches `rand_range_int()`, which is the path the assertions are written for. Checked by putting the signed `end - begin` back into `rand_range_int()`: the wide range assertions now fail there instead of being skipped.
`Addrinfo.getaddrinfo rejects out-of-range integer hints` covers the narrowing of an `mrb_int` into the C `int` of a `struct addrinfo` field, so it skips where `mrb_int` is too narrow to hold `1 << 40`. It asked that with `kind_of?(Integer)`, which a big integer answers true, so on `MRB_INT32` with mruby-bigint the test ran with a big integer hint. `Addrinfo.getaddrinfo` converts `Integer` hints only, leaves the field at its default for anything else, and the narrowing under test never runs: the test failed rather than skipping. Ask a core method taking an `mrb_int` whether the value is one, and skip unless it is. This was invisible until the file stopped being dropped at compile time on a 32-bit `mrb_int`.
Three test files stop running entirely on a build with a 32 bit
mrb_intandno bigint, and nothing says so.
An integer literal that does not fit
mrb_intis rejected while the file iscompiled, so a wide shift written out in full takes its whole file down with
it. The tests are not skipped and not failed. They are absent, and the suite
reports success with fewer tests than it had.
Counted with
MRB_INT32and no bigint, before and after this change:test/t/string.rb~(-1 << 63)mrbgems/mruby-random/test/random.rb1 << 62mrbgems/mruby-socket/test/addrinfo.rb1 << 40Total 1503 tests before, 1732 after, on a build carrying all three gems.
Every one of these lines was already written to cope with a narrow
mrb_int:None of those guards ever ran, because nothing in the file was running yet
when the value was rejected.
Fix
Take the shift width from a variable. The shift becomes a run time operation
that raises
RangeErrorwheremrb_intis too narrow, which is exactly whatthe
rescueand theskipbeside it were written for. On a build where thevalue fits, the tests run as before.
Note
This is why CI has not caught it: the
MRB_INT32builds that would show themissing tests are not part of the matrix, and a build that only counts
successes cannot tell a passing file from an absent one.
The same pattern in test additions of mine is fixed in #7113 and #7114.
Tests
MRB_INT32without bigint (stdlib, stdlib-ext, mruby-pack, mruby-sprintf,mruby-socket, mruby-io): 1732 tests, 0 failures, against 1503 before
MRB_UTF8_STRING: 2249 tests, 0 failuresSummary by CodeRabbit