Review Mojo's 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, being test_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_int
- memory/uninit_check/: 7 poison tests, all sharing the same -D flag, so they merge into one file
- builtin/: 5, being test_debug_assert_{default_error,mode_all_error,location,no_message} and test_range_len_overflow_abort
- itertools/: 2, test_{take,drop}_negative_count
- os/: 2, test_trap and test_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 (#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 #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 (#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 [BUG] [stdlib] assert_aborts is very slow (and sometimes times out) under ASAN #6912 is sorted out.
- The singletons in itertools/, builtin/, logger/, memory/ and os/.
- The two GPU tests.
Review Mojo's 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:
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:
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 (#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 #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 (#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: