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

DISC: When should and shouldn't we use pytest fixtures? #23701

Closed
jbrockmendel opened this issue Nov 14, 2018 · 7 comments
Closed

DISC: When should and shouldn't we use pytest fixtures? #23701

jbrockmendel opened this issue Nov 14, 2018 · 7 comments
Labels
Testing pandas testing functions or related to the test suite Usage Question

Comments

@jbrockmendel
Copy link
Member

pytest fixtures have a fairly specific use case, and it isn't clear to me that we are using them as intended.

The two main cases as I understand them are a) constructing something that is somewhat expensive and you would like to reuse, as you would with setup_class and b) lists of test cases that you want to write in one place instead of many. Am I missing anything major here?

Many of our fixtures don't fall into these categories: datetime_tz_utc is a singleton used exactly once, tdser just returns pd.Series(['59 Days', '59 Days', 'NaT'], dtype='timedelta64[ns]'), float_frame just returns DataFrame(tm.getSeriesData()), ...

The cost of overusing fixtures includes

  • lock-in
  • transparency: I can't look at a test and know what's being tested without looking at conftest
  • inability to run tests as stand-alone code when troubleshooting

I think at the very least we should avoid singleton fixtures, and many others can just be invoked directly from pd.util.testing instead of through fixtures.

Thoughts?

@jreback
Copy link
Contributor

jreback commented Nov 14, 2018

how’s is the 3rd point true at all?

pytest path/to/test

is all you need

@jreback
Copy link
Contributor

jreback commented Nov 14, 2018

you are missing the major benefit

consistency

we have a very large codebase and having mixed / different styles is a big -1

fixtures should have well defined named and be documented - they work as intended

@jbrockmendel
Copy link
Member Author

how’s is the 3rd point true at all?
pytest path/to/test
is all you need

That is if you want to run one test/file in isolation. But imagine you are working on a branch where some tests are causing segfaults and you want to run the test line-by-line, looking at objects in the namespace to see what is getting mutated when. Then (yah yah, pdb whatever) you want tests code that behaves like normal non-test code. Or for a more every-day example, which is clearer to the reader:

    def test_td64ser_div_numeric_scalar(self, tdser):
        expected = Series(['29.5D', '29.5D', 'NaT'], dtype='timedelta64[ns]')
        result = tdser / 2
        tm.assert_equal(result, expected)

    def test_td64ser_div_numeric_scalar(self):
        tdser = pd.Series(['59 Days', '59 Days', 'NaT'], dtype='timedelta64[ns]')
        expected = Series(['29.5D', '29.5D', 'NaT'], dtype='timedelta64[ns]')
        result = tdser / 2
        tm.assert_equal(result, expected)

I claim the latter is clearer by a wide margin.

@jbrockmendel
Copy link
Member Author

consistency
we have a very large codebase and having mixed / different styles is a big -1

By that logic we should make fixtures for every singleton used in tests, no?

@gfyoung gfyoung added Testing pandas testing functions or related to the test suite Usage Question labels Nov 14, 2018
@jreback
Copy link
Contributor

jreback commented Nov 14, 2018

if it’s truly a singleton the. no a fixture is not warranted

however i would claim that most inputs should be well designed fixtures

this prevents lots of errors and allows lots of consistency

@jbrockmendel
Copy link
Member Author

Re-opening. I thought we had consensus about singleton fixtures being unnecessary, but it now appears otherwise.

@simonjayhawkins
Copy link
Member

#24873 (comment)

@jbrockmendel I don't disagree with your reasoning for functions over fixtures. working with fixtures can be a PITA. However, I don't agree that we should restrict the use of singleton fixtures and I don't agree that we should undo work done on fixturizing tests.

I'm assuming that many of the fixtures are a result from the transition away from unittest style classes with setup methods. My opinion is that creating a fixture is a straightforward way to achieve this. Also as part of the transition test files are being split into a hierarchy of directory based tests. So my reasoning would be that a fixture placed in a confest file at the appropriate directory level is preferable than adding functions to util.testing. Hence why I would not like to see restrictions on their use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing pandas testing functions or related to the test suite Usage Question
Projects
None yet
Development

No branches or pull requests

4 participants