TST: Remove subset of singleton fixtures #24873
Conversation
@jreback you don't have to convince me of anything, clearly this won't be implemented if you're not on board. But if you want to convince me that your way makes sense, you need to lay out a coherent argument for when we should and should not use fixtures.
|
|
have a look in tests/frame and u will easily see zillions of places these should be used but are simply not yet changed over from the old syntax |
|
yes we should really get rid of the makeFoo functions - they r a holdover from a time before fixtures - the only stopping block is they r semi public Timestamp.now is too simple to be used as a fixture |
"should be used" is basically what I'm asking you to define in a non-tautological way. If "the old syntax" is "a python function", then I see no upside to the change and would really like to understand the reasoning. |
Codecov Report
@@ Coverage Diff @@
## master #24873 +/- ##
===========================================
- Coverage 92.38% 42.88% -49.51%
===========================================
Files 166 166
Lines 52412 52449 +37
===========================================
- Hits 48423 22491 -25932
- Misses 3989 29958 +25969
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24873 +/- ##
==========================================
+ Coverage 92.38% 92.39% +<.01%
==========================================
Files 166 166
Lines 52412 52449 +37
==========================================
+ Hits 48423 48461 +38
+ Misses 3989 3988 -1
Continue to review full report at Codecov.
|
|
we prefer fixtures over functions almost always:
i am just really repeating the goodness of using fixtures over functions - they are strictly better for testing singletons are not really a special case - they provide nice utility; again w/o having to have special import files scattered about - which inevitably leads to users actually calling these functions so I am -1 on this PR and any like it; we have been moving in the opposite direction for many years fixtures, organized well are a huge boon to testing sure you create singleton fixtures when you FIRST make a function into a fixture - this just means that you need to move more boilerplate code into fixtures |
i think that in a pytestonic world i assume that all the functions in tm would be in conftest, even the assert functions and that pandas.util.testing would be no more. personally i use the assert functions such as tm.assert_frame_equal to check a personal library and being able to import pandas.util.testing makes this easy.
i think that the opposite is true. the idea of fixtures is to make the testing more robust. the fixtures are functions that are run and a return value is the parameter used in the test. the default 'scope' (fixture scope is different to lexical scope etc) for a fixture is function scope which means that the fixture function is executed for each test producing a 'new' value. compared to the unittest class style using the setup method and making 'fixtures' class attributes the pytest approach is more robust. in theory we do not need to use copy() methods to 'protect' the fixture when executing tests. |
In cases where "pytestonic" and "pythonic" don't coincide, my default is to prefer pythonic. The point of the conversation (since the PR is going nowhere) is to understand why I should care about pytestonic.
So when I read through a test I don't immediately know where a name is defined? I don't see this as a plus.
I am specifically focusing on fixtures without any parametrization or composition.
Same with a function call...
Not for the ones that get inlined. Inlining For non-inlined fixtures, I'll give you this one, sort of. But there is a tradeoff with clarity. Consider two implementations of the same test. The second (my preferred) has one extra line, and so is technically more verbose. But the reader doesn't have to check what
@jreback this is a circular argument. Besides which, flake8 won't tell us when we have unused test arguments (which does occur in these files).
@jreback this. This is what I'm trying to understand.
The pythonic way to handle this is an underscore.
@simonjayhawkins The most optimistic interpretation of the disagreement is that we are not talking about the same thing, so let's double-check. I am talking about the fact that in #24769 there is a fixture has one passing and the other failing. The behavior of the first test is reachable AFAICT only via the pytest command line, which doesn't match any user's runtime environment. If it doesn't match the behavior a user sees, then it is testing behavior I couldn't care less about. If "the opposite is true", please tell me why I should care about behavior that only occurs via the pytest CLI. (and if it is not the case that it occurs only via the pytest CLI, please help me fix the extant test failure in #24769) How many other tests do we have that are silently testing behavior that doesn't match what users will actually see? @jorisvandenbossche am I missing something here? I'm trying not to rule out the possibility that jreback knows something I don't, but am still completely missing it. |
|
I haven't read through everything, but I mostly agree with
@jbrockmendel's sentiments.
In cases where we aren't using the advanced features of a fixture (setup,
teardown, parametrization), a function call should be equivalent.
And I generally find function calls easier to reason about that fixtures
(as they're easier to goto the definition if needed).
…On Tue, Jan 22, 2019 at 10:34 AM jbrockmendel ***@***.***> wrote:
in a pytestonic world
In cases where "pytestonic" and "pythonic" don't coincide, my default is
to prefer pythonic. The point of the conversation (since the PR is going
nowhere) is to understand why I should care about pytestonic.
you don’t have one giant file with imports of these
So when I read through a test I don't immediately know where a name is
defined? I don't see this as a plus.
they provide nice composition with other fixtures and parameters
I am specifically focusing on fixtures without any parametrization or
composition.
a test writer doesn’t have to worry about side effect / test setup or
teardown
Same with a function call...
they less verbose than a full function call online the test
Not for the ones that get inlined. Inlining empty_frame = DataFrame({})
is way less verbose than a fixture.
For non-inlined fixtures, I'll give you this one, sort of. But there is a
tradeoff with clarity. Consider two implementations of the same test.
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)
The second (my preferred) has one extra line, and so is technically more
verbose. But the reader doesn't have to check what tdser means or dig up
where it was defined.
they live in the arguments as a fixture should
@jreback <https://github.com/jreback> this is a circular argument.
Besides which, flake8 won't tell us when we have unused test arguments
(which does occur in these files).
i am just really repeating the goodness of using fixtures over functions -
they are strictly better for testing
@jreback <https://github.com/jreback> this. This is what I'm trying to
understand.
which inevitably leads to users actually calling these functions
The pythonic way to handle this is an underscore.
i think that the opposite is true. the idea of fixtures is to make the
testing more robust. [... juxtaposition with unittest class style]
@simonjayhawkins <https://github.com/simonjayhawkins> The most optimistic
interpretation of the disagreement is that we are not talking about the
same thing, so let's double-check. I am talking about the fact that in
#24769 <#24769> there is a
fixture float_frame and an identical function get_float_frame such that
def test_foo(self, float_frame):
[...]
def test_foo2(self):
float_frame = get_float_frame()
[...]
has one passing and the other failing. The behavior of the first test is
reachable AFAICT *only* via the pytest command line, which doesn't match
*any* user's runtime environment. If it doesn't match the behavior a user
sees, then it is testing behavior I couldn't care less about.
If "the opposite is true", please tell me why I should care about behavior
that *only* occurs via the pytest CLI. (and if it is not the case that it
occurs only via the pytest CLI, please help me fix the extant test failure
in #24769 <#24769>)
How many other tests do we have that are silently testing behavior that
doesn't match what users will actually see?
@jorisvandenbossche <https://github.com/jorisvandenbossche> am I missing
something here? I'm trying not to rule out the possibility that jreback
knows something I don't, but am still completely missing it.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#24873 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIlbNMWPmyh1QBf8durCX5lKtOna-ks5vFz2VgaJpZM4aL1xB>
.
|
gfyoung
added
Refactor
I agree with this sentiment as well. That being said, I have to agree that we probably haven't given these fixtures enough of a chance to be used elsewhere in the codebase. In addition, I wonder if we really should be placing ALL of these functions in |
|
BTW, I would like to remind everyone that @jbrockmendel had this original issue (#23701) around this exact topic that he recently re-opened. I think we should continue the conversation there if we are currently at an impasse. |
|
@jbrockmendel your example doesn't make much sense. these should be identically, but we prefer the first is much more informative, scopes very nicely, plays nicely with fixtures, doesn't have the test writer worry about setup/teardown. if you write tests and run them using |
interestingly, i've only just realised that there is only one fixture in the codebase that is used with '@pytest.mark.parametrize(..., indirect=True)..... and i'm guilty! the case where i've used it is not a good example to demonstrate the power of this capability. perhaps some of the singleton fixtures in question should be constructed to allow indirect parametrization and more benefit could then be gained from using fixtures. |
|
Late to the discussion but FWIW also -1 on the change, mostly citing this comment |
|
I am also following @jbrockmendel reasoning here. As Tom said, fixtures are really cool for the more advanced cases (ones that yield with a setup/teardown, parametrized ones, tempdir, ...). So I would be +1 on a more selective use of fixtures. |
@jbrockmendel do we already understand why they are not identical (there were occasional failures?)
I think we are talking here about fixtures with simple data generation, so in that case there is no setup/teardown involved, it wouldn't need to integrate with other fixtures, .. (and personally I would argue it is also not more informative, but rather less, as the function-way shows where the function is coming from). |
@jorisvandenbossche I don't have a handle on it. @simonjayhawkins is helping troubleshoot it over in #24769. Haven't given up on figuring it out, but in the interim I'm counting this as a point against unnecessary fixturization. @WillAyd you reference this comment, can I ask what part of that you find compelling? I addressed it here.
@gfyoung I'm not quite clear on what you mean by this. Any singleton fixture changed into a regular function can still be used elsewhere in the codebase. @jreback re:
"should behave identically" --> yah, they should. and the fact that they dont in at least one case suggests we should be testing the one that matches users runtime envirionment. "first is much more informative" --> in what way? The only difference in the code is that it is less obvious where the thing is defined. "scopes very nicely" --> honestly not sure what this means "plays nicely with fixtures" --> again, this is a circular argument. "doesn't have the test writer worry about setup/teardown" --> again, this is no different from a regular function "run them using pytest" --> usually I run them using pytest. but when one fails I want to be able to copy/paste it line-by-line in the REPL. And again, we have at least one case in which the called-via-pytest behavior doesn't match the regular-python-function behavior. This is bad.
Zen of python ring a bell? |
My point is that while I agree that truly singleton fixtures should be replaced, I'm not sure the current fixtures that we're removing are truly singletons, given how non-pytest-idiomatic some of the codebase still is.
This sounds like you just don't like fixtures in any way, eh? 😉 BTW, I should point out that REPL isn't the only way to debug these tests (print statements, |
Huh?
I've come around to understand their usefulness in specific use cases. These are not those cases. |
I think we are again speaking about a different sort of "singleton" ? (see #24769 (comment)) |
|
Regarding singletons-or-not, and the larger context of dependency injection in the frame/series tests (currently mostly through
That being said, I do find it hard to parse if a test requires specific properties of a fixture to work (e.g. specific column names or values), where one has to then go check in the respective In that sense, I'd argue for fixtures that are minimal to the requirements of the test they're used in. If that means they're module-specific (and get moved there!), that's fine for me. Fixtures are excellent for parametrisation though, and we should probably use a whole lot more of that, to increase breadth and the chance to catch edge cases early. OTOH, if widespread fixturization is not seen as a positive, then #22471 #22550 should be re-discussed and maybe closed. |
|
@h-vetinari I have no objection to moving away from the class attributes usage. My problems are specifically with
Fixtures which actually use any of the useful features of pytest.fixture, e.g. parametrization, are orthogonal to the issue/PR(s) |
|
@jbrockmendel I'll note that while those PRs/issues have me as the OP, the fixturisation was orthogonal to my goal with #22236, and is the result of review requirements. |
Broken off of #24769 where we have learned that some test behavior depends on whether or not a fixture is being used.
I claim that this is another point in favor of not using fixtures if there is a regular-python alternative (in this case, a "function"). Whenever with-fixture behavior is different from without-fixture behavior, it is definitely the latter that better represents user runtime environments. That is what we should be testing.