[cub] Replace assert with CHECK or REQUIRE in tests#8999
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
important: WalkthroughTest-only changes: device- and host-side C important: ChangesCatch2 assertion framework migration
important: Possibly related PRs
important: Suggested reviewers
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0274cd35-7d0c-401a-865f-079699d7dda5
📒 Files selected for processing (8)
cub/test/catch2_test_block_load_to_shared.cucub/test/catch2_test_block_radix_sort_custom.cucub/test/catch2_test_device_decoupled_look_back.cucub/test/catch2_test_device_segmented_scan.cucub/test/catch2_test_warp_scan.cucub/test/catch2_test_warp_scan_api.cucub/test/catch2_test_warp_scan_partial_helper.cuhcub/test/internal/catch2_test_integer_utils.cu
f6b8406 to
329af26
Compare
| @@ -1,142 +0,0 @@ | |||
| // SPDX-FileCopyrightText: Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
| // SPDX-License-Identifier: BSD-3-Clause | |||
| #include <cub/detail/integer_utils.cuh> | |||
There was a problem hiding this comment.
I've removed the source file & test. The functionality is not used anywhere in cub, plus it was never actually tested, because the assert expanded to noop instead of the test due to missing #undef NDEBUG
This comment has been minimized.
This comment has been minimized.
| CHECK(thread_keys[threadIdx.x][0] == expected_output[threadIdx.x][0]); | ||
| CHECK(thread_keys[threadIdx.x][1] == expected_output[threadIdx.x][1]); | ||
| CHECK(thread_keys[threadIdx.x][2] == expected_output[threadIdx.x][2]); |
There was a problem hiding this comment.
@gevtushenko is strongly for using REQUIRE here and in the rest of the PR
There was a problem hiding this comment.
I remain against. As a person that has to figure out what's going on in case of a test failure, I need the largest context possible of meaningful information.
That means, I don't want to see dependent failures, like the failed comparison with an expected result after a failed call to radix sort. That call to radix sort needs a REQUIRE of course. But a comparison of the results keys and values needs a CHECK since in case of a mismatch in the result, I want to see the mismatch of the keys and values. This may allow me to deduce better, what kind of failure happened, without first modifying the test code and recompiling.
@gevtushenko what would be the advantage of REQUIRE here? I don't see it.
There was a problem hiding this comment.
To add some context from the code review hour: According to @davebayer CHECK in device code is currently implemented to do the same as REQUIRE because anything else would be hard to implement. So with the current implementation you get REQUIRE semantics anyway and @gevtushenko just argued that this state is confusing and that CHECK should not compile in device code if it can't provide the correct semantics.
There was a problem hiding this comment.
I see. I didn't see this was device code. Fine then!
329af26 to
c9b107a
Compare
🥳 CI Workflow Results🟩 Finished in 1h 11m: Pass: 100%/285 | Total: 2d 08h | Max: 42m 54s | Hits: 89%/217105See results here. |
Using
assertoutside of libcu++ for testing is not safe, because we compile with optimizations enabled and that meanstNDEBUGis defined, thusassertexpands to noop. This PR replaces them with catch2 test macros.