[Feature Request] [stdlib] Migrate the remaining abort tests to _assert_aborts
Nobody has claimed this yet.
- Dominant language
- Mojo
- Stars
- 29.8k
- Forks
- 3.2k
- PR merge metrics
- No merged PRs in 30d
Description
Review Mojo's priorities
- I have read the roadmap and priorities and I believe this request falls within the priorities.
What is your request?
std.testing._assert_aborts() replaced the old -D test=N + lit RUN-line pattern for testing code that aborts. I converted mojo/stdlib/test/collections/test_span_bounds_abort.mojo along with it, but about 40 other stdlib tests still use one of the two older patterns. This tracks moving them over.
The first group is the lit tests that dispatch on -D test=N through a comptime if get_defined_int["test"]() chain, one RUN line per case:
| File | Cases |
|---|---|
mojo/stdlib/test/collections/test_list_bounds_abort.mojo |
5 |
mojo/stdlib/test/collections/string/test_string_span_bounds_abort.mojo |
4 |
mojo/stdlib/test/collections/string/test_string_span_codepoint_grapheme_bounds_abort.mojo |
9 |
mojo/stdlib/test/memory/test_fail/layout_invalid_alignment_abort.mojo |
2 |
mojo/stdlib/test/memory/test_fail/alloc_zst_fail_tests.mojo |
1 |
mojo/stdlib/test/collections/string/test_string_unicode_panic.mojo (argv dispatch, not not %t) |
6 |
Each RUN line becomes a test_* function, and the files become plain mojo_test targets that drop out of _LIT_TESTS in their BUILD file.
The second group is tests that get a whole mojo_filecheck_test(expect_crash = True) target for a single abort. There are 34:
collections/: 15, beingtest_negative_index_*(9),test_check_bounds_{assert_error,direct_call},test_binary_heap_assert_empty_{pop,peek},test_optional_none_abort,test_list_getitem_invalid_index_intmemory/uninit_check/: 7 poison tests, all sharing the same-Dflag, so they merge into one filebuiltin/: 5, beingtest_debug_assert_{default_error,mode_all_error,location,no_message}andtest_range_len_overflow_abortitertools/: 2,test_{take,drop}_negative_countos/: 2,test_trapandtest_trap_gpu- one each in
logger/,memory/,memory/pointer/
The 15 in collections/ collapse into one file, as do the 7 in uninit_check/. That pair alone is 22 bazel targets down to 2.
What is your motivation for this change?
A single bounds-check assertion currently costs either a RUN line plus a branch in a comptime dispatch chain, or a source file and a bazel target of its own. That is a lot of ceremony for one assertion, and it keeps these checks out of the file where the rest of the type's tests live, so nobody reads them together.
Any other details?
ASAN is the thing to get right. _assert_aborts early-returns under SanitizeAddress (modular/modular#6912), so converting a target that runs under ASAN today drops that assertion from the ASAN build with nothing to flag it. memory/uninit_check/, memory/pointer/ and the memory/test_fail/ lit tests are already ASAN-excluded and convert for free. collections/, builtin/, itertools/, logger/, memory/ and os/ are not.
There is no equivalent of CHECK-NOT. The common CHECK-NOT: is never reached is covered by the process aborting, but logger/test_logger_critical.mojo checks that no DEBUG or INFO output appears ahead of the critical, and that would be lost. A not_contains= argument would cover it; I'll file that under modular/modular#6908.
Each case needs its own call site. _assert_aborts tells children apart by call_location(), so looping over a table of cases only ever tests the first iteration (modular/modular#6913).
The remaining wrinkles are per-file. -D BUILD_TYPE=debug, -D ASSERT=... and MOJO_STDLIB_SIMD_UNINIT_CHECK decide which files can share a target. test_debug_assert_location.mojo asserts on file:line:col, so the expected line has to be matched explicitly or the test gets weaker. The two GPU tests would re-exec a GPU binary per case.
The leading underscore isn't a reason to wait. _assert_aborts is exported from std.testing and usable now, and if it loses the underscore later that's a rename across the test tree.
Order I'd go in, each step landable on its own:
- The six files in the first group. The three bounds-checking files in
collections/convert almost line for line and are a good place to start. memory/uninit_check/, 7 targets to 1, already ASAN-excluded.- The 15 in
collections/, once modular/modular#6912 is sorted out. - The singletons in
itertools/,builtin/,logger/,memory/andos/. - The two GPU tests.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the three collections bounds-checking files listed in the first group and read the existing _assert_aborts usage in test_span_bounds_abort.mojo. Convert each case into its own test function, update the related BUILD targets, and run the affected stdlib tests. Done means the old -D test=N and lit RUN-line patterns are removed while ASAN behavior and per-case abort checks remain covered.
Written by the indexing model from the issue text.
Assessment
- Domain
- build-system, compilers, testing-qa
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 52/100