[Freeipa-devel] [RANT] pytest fixture scope is braindead

[Nathaniel McCallum]

On Mon, 2014-12-08 at 12:27 +0100, Petr Viktorin wrote:
> On 12/05/2014 11:51 PM, Nathaniel McCallum wrote:
> > So I've been working this week on porting the OTP tests that were
> > manually coded to the new pytest framework. Now, I'm the first to want
> > better tooling to make our job easier. But, pytest? Meh. I'm having to
> > write just as much code (or more) to get my tests on par with pytest,
> > and they are riddled with subtle bugs.
> 
> Hi,
> Yeah, IPA runs into specific situations that are very different from how 
> tests usually work in other projects.

Yeah, this I get.

> And you took one of the harder cases.
> IPA's existing tests also don't really follow best practices – they 
> pretty much assume you run the whole test suite, or at least the whole 
> module, so selecting individual tests is currently not reliable. On the 
> other hand, this assumption made some things easy to code – like 
> complicated ordering and setup/teardown.

I think this is pretty much a red herring. It isn't so much about setup
and teardown but about mutually exclusive parameterization. It is a poor
assumption on the part of pytest that the lifecycles of parameters
overlap. At the least, an option should be provided to control this.

> One thing we want to do during the move to pytest is to have the tests 
> be more independent of the current state of the database, and also of 
> each other.

Sure, this is a fine goal. Unless of course you want to test the
functionality of the code against all possible database states. I think
pretty much every xmlrpc test will have some component of this.

> If certain assertions only make sense in a specific order, either put 
> them in the same test*, or write additional code that explicitly handles 
> dependencies and ordering. (Sorry, that's the price to pay for isolated 
> tests – I don't see a way around it.)

In an ideal world, we define a test and all possible parameters to that
test and allow the running of that test independently by choosing from
the proscribed inputs. This all works fine so long as you specify a way
to indicate when inputs are mutually exclusive. Since inputs are single
valued anyway, this should be the default behavior. Unfortunately,
pytest fails on both of these counts.

> If on the other hand the order doesn't matter – any order of any subset 
> of individual tests should pass, which is our goal – but pytest's 
> default ordering makes the suite slow, there is a hook that allows you 
> to override the order: pytest_collection_modifyitems [0]. Stick it in 
> the affected module (or conftest.py file for multiple modules, or a 
> plugin for the whole suite), and sort the items explicitly.

The problem isn't (I think) ordering but lifecycle. I think that if I
scope the fixture to the function but reorder the items, they will still
be constructed and torn down for every single function iteration. On the
other hand, if I scope the fixture for a longer lifecycle, the
parameters may not be mutually exclusive.

> If your test depends on something not existing or being disabled, you 
> should try removing/disabling it before the test, or write a fixture to 
> do that.

That's not the problem. The problem is that I can't define parameters
for a single object which sets the same variable to different values.
Such an act means the parameters are mutually exclusive.

> * might make sense, for starters?
> [0] 
> http://pytest.org/latest/plugins.html#_pytest.hookspec.pytest_collection_modifyitems
> 
> 
> > Most painful, is pytest.fixture(). It is a cool idea, but the scoping
> > and lifecycle is horribly broken. Here is an example:
> >
> > Here is an example. I want to create a user, enable otp in two different
> > ways, test with two different types of tokens under two different
> > authentication scenarios. This is a total of 8 tests.
> >
> > @pytest.fixture(scope="function") # This scope is default.
> > def user(request):
> >    user = create_user('tuser1')
> >    request.addfinalizer(lambda: delete_user('tuser1'))
> >    return user
> >
> > @pytest.fixture(params=["per-user", "global"])
> > def enablement(request, user):
> >    output = enable(user, request.param)
> >    request.addfinalizer(lambda: disable(user))
> >    return output
> >
> > @pytest.fixture(params=[{'type': 'HOTP'}, {'disabled': True}])
> > def token(request, user, enablement):
> >    output = create_token(request.param)
> >    request.addfinalizer(lambda: delete_token(output.id))
> >    return output
> >
> > @pytest.mark.parametrize("opts", [foo, bar])
> > def test_authentication(user, token, opts):
> >    return auth(user.uid, token.code(), opts)
> >
> > Because the default scope is "function", a new user is created for every
> > single run of token. This is *way* more work than necessary, and leads
> > to slow runtimes. Fortunately, we can fix this by changing the scope of
> > the user fixture to "module". In this case, only one user will be
> > created per module. So far so good.
> >
> > The enablement scope is still "function", so it will still be called
> > eight times (#tokens * #enablements * #opts). Now, this is a problem
> > because (let's say) enablement takes a long time to switch tests. Here
> > is problem #1 with pytest: it presumes that all variants of fixtures
> > should be grouped by test parameters. This is a bad assumption. It may
> > make sense in some cases, but there is no way to change it.
> >
> > Now, we can work around this problem just like with the user fixture:
> > change the scope to "module". Unfortunately, we've just caused a second
> > bug. Now, enablement() will only be run twice; but the finalizer will
> > not be called until all tests are run. This means that "per-user"
> > enablement will still be active when "global" enablement runs; both will
> > be torn down simultaneously.
> >
> > This same problem arises for the token fixture: eight tokens will be
> > created and destroyed when only four are necessary. You guessed it, we
> > can fix this by changing the scope. But by doing this all four tokens
> > will be deleted simultaneously. This means we cannot test the state
> > where only a disabled token exists.
> >
> > Any attempt to use a fixture to create long-lived states (precisely what
> > fixtures are designed for) with mutually exclusive parameters is
> > extremely error prone as it is not clear which items are alive in the
> > LDAP db at any point. This leads to both long runtimes *and* extremely
> > subtle bugs in the tests that are multiplied by every layer of fixtures.
> > Call me nonplussed.
> >
> > Now, let's look at how this same test can be implemented without pytest:
> >
> > user = create_user('tuser1')
> > try:
> >    for enablement in ('per-user', 'global'):
> >      enable(user, enablement)
> >      try:
> >        for token in [{'type': 'HOTP'}, {'disabled': True}]:
> >          object = create_token(token)
> >          try:
> >            for opt in [foo, bar]:
> >              auth(user.uid, object.code(), opt)
> >          finally:
> >            delete_token(object.id)
> >      finally:
> >        disable(user)
> > finally:
> >    delete_user('tuser1')
> >
> > Comparing the non-pytest implementation to the pytest one we can see a
> > few things. First, the pytest code has some nice separation of roles.
> > This is the big benefit of pytest. Second, the non-pytest implementation
> > is less code. Third, the non-pytest implementation has no subtle scoping
> > bugs: we know exactly when each database record is created and
> > destroyed.
> 
> This "non-pytest" implementation is a *single test*, since it hard-codes 
> dependencies and ordering. If you write it as as a single test function 
> in pytest, it should look much the same, and get the 
> ordering/dependencies right (except the case of the test user already 
> existing).

Which conflicts with the desired granularity; which is the whole reason
we are moving away from this model. I should be able to define, as
fixtures, a set of states required to run any one of a set of test. Then
I can choose just to run a single iteration of a set of tests and the
set of states should be assembled correctly. The problem is that, with
pytest, this all works unless the parameterized fixtures contain
mutually exclusive state values.

So, I'm obviously not proposing that we do something like the non-pytest
implementation. What I'm pointing out is that being forced to implement
it this way in pytest negates all the benefits of pytest.

> > I *want* to like pytest. Honestly I do. But this scoping idiocy has
> > already cost me roughly two days tracking down subtle bugs in the tests
> > (while not finding any bugs in the OTP code itself). And I can't think
> > that OTP tests written using pytest will be maintainable in any regard;
> > not when such small changes can cause such hard to find bugs.
> >
> > Now, it may be possible that I have just missed something obvious. If I
> > have, forgive me this rant. But I've spent a lot of time reading docs
> > and I haven't seen any obvious solutions to these problems.
> >
> > Nathaniel
>