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

TST: Remove groupby/test_function.py #56338

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

rhshadrach
Copy link
Member

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

This finishes up the removal of test_function.py from the groupby tests. With the exception of test_groupby_non_arithmetic_agg_int_like_precision (which is moved to other files here), the tests that remained fall into two (and partially overlapping) camps:

  • Tests for numeric_only
  • Tests that use the fixture groupby_func, which goes over every groupby operation

For the first, the options I considered were creating test_numeric_only.py or split it up into various other files such as test_reductions.py, test_cumlative.py, and some others. Similarly, for tests that use groupby_func that don't fit in other files, I considered creating test_all_methods.py (which can be renamed to something better) or split up the tests into several different files.

I'm completely on the fence here. Breaking up a parameterized test and duplicating code across multiple files seems pretty bad. While I don't love having test_all_methods (it's sort of a catch-all), at first it seemed to me that it is less bad than breaking up the parameterized tests. But then grepping around, it appears that there are only three tests across all of groupby that would go in test_all_methods. Perhaps this is something we can expand on later - but maybe since there are so few tests it's okay to break these up?

Any ideas here are very much welcome.

cc @jbrockmendel @mroeschke

@rhshadrach rhshadrach added Testing pandas testing functions or related to the test suite Groupby Clean labels Dec 5, 2023
(24650000000000001, 24650000000000002),
],
)
def test_groupby_nth_int_like_precision(data):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests has been cleaned up and not just moved.

],
)
@pytest.mark.parametrize("method", ["count", "min", "max", "first", "last"])
def test_groupby_non_arithmetic_agg_int_like_precision(method, data):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests has been cleaned up and not just moved.

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable to me. nice work

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking up a parameterized test and duplicating code across multiple files seems pretty bad. While I don't love having test_all_methods (it's sort of a catch-all), at first it seemed to me that it is less bad than breaking up the parameterized tests.

IMO I think in the future this could be rolled into a potential refactor of test_groupby.py as a file testing "groupby scenarios that should work with all methods"

@mroeschke mroeschke added this to the 2.2 milestone Dec 5, 2023
@mroeschke mroeschke merged commit dc4c474 into pandas-dev:main Dec 5, 2023
50 checks passed
@mroeschke
Copy link
Member

Thanks @rhshadrach

@rhshadrach
Copy link
Member Author

Breaking up a parameterized test and duplicating code across multiple files seems pretty bad. While I don't love having test_all_methods (it's sort of a catch-all), at first it seemed to me that it is less bad than breaking up the parameterized tests.

IMO I think in the future this could be rolled into a potential refactor of test_groupby.py as a file testing "groupby scenarios that should work with all methods"

Sounds good - test_groupby is my next target; I've always been under the impression that tests here should be for something like "All groupby attributes that aren't ops and don't have a more specific location". E.g. len(df.groupby(...)) or df.groupby(...).groups. I think it makes sense to roll test_all_methods into here.

@rhshadrach rhshadrach deleted the clean_gb_tests_7 branch December 5, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Groupby Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants