Skip to content

Conversation

@ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Nov 24, 2025

Currently, many groupby tests call to test_single_agg() and other utility functions in groupby_test_util.* (test_sum_agg() and test_single_scan()) to execute the test. When a failure happens, GTest only issues a trace location to somewhere in test_single_agg() while there is not any information about the actual failed test. That makes debugging a bit tricky.

This PR reimplements thesef test functions, adding one more parameter std::source_location const& location = std::source_location::current() to carry the caller's information. With this, GTest will report an additional line containing the location of the failed test. For example:

Google Test trace:
cudf/cpp/tests/groupby/groupby_test_util.cpp:75:  <--  line of failure

cudf/cpp/tests/groupby/groupby_test_util.cpp:36: Original failure location: cudf/cpp/tests/groupby/sum_tests.cpp:192

Signed-off-by: Nghia Truong <[email protected]>
Signed-off-by: Nghia Truong <[email protected]>
@ttnghia ttnghia self-assigned this Nov 24, 2025
@ttnghia ttnghia requested a review from a team as a code owner November 24, 2025 22:51
@ttnghia ttnghia added the 3 - Ready for Review Ready for review by team label Nov 24, 2025
@ttnghia ttnghia added the tests Unit testing for project label Nov 24, 2025
@ttnghia ttnghia added this to libcudf Nov 24, 2025
@ttnghia ttnghia added libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 24, 2025
Signed-off-by: Nghia Truong <[email protected]>
Signed-off-by: Nghia Truong <[email protected]>
@pmattione-nvidia
Copy link
Contributor

Instead of creating macros and modifying hundreds of call sites, you should just be able to add a default argument to the end of each method of std::source_location. The example in that link shows how you can default the argument to std::source_location::current() and it will capture the same information.

@ttnghia
Copy link
Contributor Author

ttnghia commented Dec 1, 2025

Instead of creating macros and modifying hundreds of call sites, you should just be able to add a default argument to the end of each method of std::source_location. The example in that link shows how you can default the argument to std::source_location::current() and it will capture the same information.

That is great. I've modified the code using that approach.

Signed-off-by: Nghia Truong <[email protected]>
@ttnghia ttnghia force-pushed the improve_groupby_test branch from 6e8bd08 to 02f498a Compare December 2, 2025 17:41
@davidwendt
Copy link
Contributor

This looks great.
Can you add a comment which shows how the output would look now on a failure with this implementation?

@ttnghia
Copy link
Contributor Author

ttnghia commented Dec 2, 2025

This looks great. Can you add a comment which shows how the output would look now on a failure with this implementation?

Yes, the output should look the same as reported in the PR description: #20718 (comment).

@ttnghia
Copy link
Contributor Author

ttnghia commented Dec 2, 2025

/merge

@rapids-bot rapids-bot bot merged commit dd12a0f into rapidsai:main Dec 2, 2025
522 of 529 checks passed
@ttnghia ttnghia deleted the improve_groupby_test branch December 2, 2025 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change tests Unit testing for project

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants