Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor tests/groupby/** #7604

Merged
merged 3 commits into from
May 6, 2021

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Mar 15, 2021

No change in code/implementation by this PR. It only renames the test files in the folder tests/groupby. In particular:

  • It removes the prefixes group_ and groupby_ from all the file names in that folder.
  • The file suffix _test.cpp is changed to _tests.cpp to enforce consistency with other test files.

The reasons for doing this PR are:

@ttnghia ttnghia requested review from a team as code owners March 15, 2021 23:22
@ttnghia ttnghia requested review from trxcllnt and jrhemstad March 15, 2021 23:22
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Mar 15, 2021
@ttnghia ttnghia added non-breaking Non-breaking change tests Unit testing for project CMake CMake build issue 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function and removed CMake CMake build issue labels Mar 15, 2021
@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #7604 (8e3a999) into branch-0.20 (51336df) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.20    #7604      +/-   ##
===============================================
+ Coverage        82.88%   82.91%   +0.02%     
===============================================
  Files              103      103              
  Lines            17668    17863     +195     
===============================================
+ Hits             14645    14811     +166     
- Misses            3023     3052      +29     
Impacted Files Coverage Δ
python/cudf/cudf/core/abc.py 91.66% <ø> (+0.17%) ⬆️
python/cudf/cudf/core/algorithms.py 82.35% <ø> (ø)
python/cudf/cudf/core/column/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/core/column/categorical.py 92.37% <ø> (+0.13%) ⬆️
python/cudf/cudf/core/column/column.py 88.20% <ø> (-0.44%) ⬇️
python/cudf/cudf/core/column/datetime.py 88.03% <ø> (-1.88%) ⬇️
python/cudf/cudf/core/column/decimal.py 90.29% <ø> (-2.64%) ⬇️
python/cudf/cudf/core/column/interval.py 91.66% <ø> (+0.55%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 80.42% <0.00%> (-4.11%) ⬇️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7623f39...8e3a999. Read the comment docs.

@ttnghia
Copy link
Contributor Author

ttnghia commented Mar 16, 2021

Rerun tests.

@devavret
Copy link
Contributor

  • Their names (group_ and groupby_) already show such inconsistency.

This is intentional. group_ prefix is for the aggregation. group_max_test is a test that checks our ability to get each "group's max". groupby_ prefix is only meant for keys. It is a test of our ability to "group by different key types". The only inconsistency is in groupby_groups_test which was added later.

@ttnghia
Copy link
Contributor Author

ttnghia commented Mar 16, 2021

  • Their names (group_ and groupby_) already show such inconsistency.

This is intentional. group_ prefix is for the aggregation. group_max_test is a test that checks our ability to get each "group's max". groupby_ prefix is only meant for keys. It is a test of our ability to "group by different key types". The only inconsistency is in groupby_groups_test which was added later.

Do you mean only groupby_groups_test needs to be renamed, the other files must remain intact?

@devavret
Copy link
Contributor

the other files must remain intact?

I was just explaining the rationale behind the original names. If they still make sense with a smaller name then we should have a smaller name.

@mythrocks
Copy link
Contributor

I think I should push this ASAP so other people can sync ASAP.

Curious: Why is this urgent?

@ttnghia
Copy link
Contributor Author

ttnghia commented Mar 16, 2021

Not "urgent", just in case somebody is adding new test files to this folder then they can avoid adding those prefixes, so I can avoid refactoring again their files in the future when I merge my PR.

@ttnghia
Copy link
Contributor Author

ttnghia commented Mar 16, 2021

the other files must remain intact?

I was just explaining the rationale behind the original names. If they still make sense with a smaller name then we should have a smaller name.

Thanks for your comment. Then we need to change the file names, since those files are already in the groupby folder, which means they belong to groupby.

@mythrocks
Copy link
Contributor

Not "urgent", just in case somebody is adding new test files...

I wonder if it might be acceptable to reserve aesthetic/compliance changes for branch-0.20, given how close we are to branch-0.19 burn-down.
This sort of thing will cause last minute churn for those making material changes to group-by tests.

@ttnghia
Copy link
Contributor Author

ttnghia commented Mar 16, 2021

I don't think it will affect anything, as it only renames files instead of changing any implementation.

@ttnghia
Copy link
Contributor Author

ttnghia commented Mar 16, 2021

Moved this PR to v0.20 release to avoid extra work for the team as v0.19 is near its burn-down stage.

@karthikeyann
Copy link
Contributor

After #7387, this PR require more tests files renamed.

# Conflicts:
#	cpp/tests/CMakeLists.txt
@ttnghia ttnghia self-assigned this Apr 25, 2021
# Conflicts:
#	cpp/tests/CMakeLists.txt
@ttnghia ttnghia requested a review from a team as a code owner May 3, 2021 15:20
@ttnghia ttnghia requested review from a team as code owners May 3, 2021 15:20
@ttnghia ttnghia requested review from cwharris and removed request for a team May 3, 2021 15:20
@ttnghia ttnghia changed the base branch from branch-0.19 to branch-0.20 May 3, 2021 15:21
@ttnghia ttnghia removed request for a team May 3, 2021 15:22
@ttnghia ttnghia removed the CMake CMake build issue label May 3, 2021
@ttnghia ttnghia requested review from mythrocks and removed request for trxcllnt and jrhemstad May 5, 2021 20:08
@harrism harrism added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels May 6, 2021
@harrism
Copy link
Member

harrism commented May 6, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e8b9ff7 into rapidsai:branch-0.20 May 6, 2021
@ttnghia ttnghia deleted the refactor_groupby_test branch May 27, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change tests Unit testing for project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants