fix: resetting state modifying fixtures not function scoped

[skellet0r]

What I did

Remove chain snapshotting prior to fixture execution/setup, and create a ape_test fixture function_isolation which does the behavior presumably desired (snapshot prior to tests and reset after).

fixes: #653
fixes: APE-165

How I did it

Simple, copy the original pytest_runtest_protocol function body and paste it into the function_isolation fixture.

How to verify it

Check the added test case, also can be verified against the test project in issue #653

Checklist

• All changes are completed
• New test cases have been added
• Documentation has been updated

[antazoey]

Some small formatting issues, but this works great and I am glad it was an easy fix. Thank you for contributing!

[skellet0r]

@fubuloubu @unparalleled-js I guess the next question is, beyond function scope isolation, do you think module level isolation should be implemented here as well? It may lead to needing isolation by default for all scopes 🤔

[fubuloubu]

@fubuloubu @unparalleled-js I guess the next question is, beyond function scope isolation, do you think module level isolation should be implemented here as well? It may lead to needing isolation by default for all scopes thinking

At least supporting "module" and "function" scopes would be nice. "session" scopes should be left only for accounts basically, but supporting them more generally is important.

[fubuloubu]

Looking good! Verifying the action of this change locally before merging

[skellet0r]

@fubuloubu Actually 🤔 second guessing myself here but is snapshotting right before the test is ran ok?

Using brownie for example, the function isolation fixture runs first at the function level:

        SETUP    F function_isolation (fixtures used: chain, history, module_isolation)
        SETUP    F alice_unlock_time (fixtures used: chain)
        SETUP    F lock_alice (fixtures used: alice, alice_lock_value, alice_unlock_time, crv, vecrv)
        SETUP    F timedelta_bps[60]
        tests/boosts/test_batch_cancel_boosts.py::test_third_parties_can_only_cancel_past_expiry[60] (fixtures used: VotingEscrowDelegation, accounts, alice, alice_lock_value, alice_unlock_time, bob, chain, charlie, crv, function_isolation, history, lock_alice, module_isolation, multicall, pm, timedelta_bps, veboost, vecrv)
        TEARDOWN F timedelta_bps[60]
        TEARDOWN F lock_alice
        TEARDOWN F alice_unlock_time
        TEARDOWN F function_isolation

But this PR would change it to having snapshotting right before the test is ran, in brownie that would look like the isolation fixture is run last

Edit: The effect is, if a enduser does any state changes in a function level fixture, those will not be reset in the development chain. i.e. if I deploy Foo in a function level fixture, it'll be redeployed for as many tests as I have

[fubuloubu]

if a enduser does any state changes in a function level fixture, those will not be reset in the development chain. i.e. if I deploy Foo in a function level fixture, it'll be redeployed for as many tests as I have

This seems like desired behavior. If you want to snapshot things after initial setup, use "session". If you want to snapshot each test module, use "module". And if you need something to restart each test (let's say for a contract deployment that depends on timestamp or something) use "function"

Edit: I know Brownie works differently, but I want this behavior so that we hook more deeply into how pytest already works, so that there is no context switching between ape-flavored pytest fixtures and regular pytest fixtures

[skellet0r]

This seems like desired behavior. If you want to snapshot things after initial setup, use "session". If you want to snapshot each test module, use "module". And if you need something to restart each test (let's say for a contract deployment that depends on timestamp or something) use "function"

Was inspecting this some more (i.e. snapshotting by default around scopes), and my initial idea was to obviously find whatever hook pytest has for setting up and tearing down fixtures (which it does).

The downside is, the way fixture setup is done in pytest, a single call is done to the hook for each test (as opposed to a call to the hook per fixture): https://github.com/pytest-dev/pytest/blob/main/src/_pytest/fixtures.py#L1096-L1118

So off the top of my head I'm thinking as a dirty tactic, we could re-implement the function body in the Ape runner, overriding pytest's default, and then do the snapshot and resetting within it.

cc: @fubuloubu @unparalleled-js

Edit: there's also the option of fixtures, but must concur, snapshotting by default is most reasonable
Related: pytest-dev/pytest#3677

Looks like maybe internal fixtures is the way to go (whatever that may mean)

[fubuloubu]

This seems like desired behavior. If you want to snapshot things after initial setup, use "session". If you want to snapshot each test module, use "module". And if you need something to restart each test (let's say for a contract deployment that depends on timestamp or something) use "function"

Was inspecting this some more (i.e. snapshotting by default around scopes), and my initial idea was to obviously find whatever hook pytest has for setting up and tearing down fixtures (which it does).

The downside is, the way fixture setup is done in pytest, a single call is done to the hook for each test (as opposed to a call to the hook per fixture): https://github.com/pytest-dev/pytest/blob/main/src/_pytest/fixtures.py#L1096-L1118

So off the top of my head I'm thinking as a dirty tactic, we could re-implement the function body in the Ape runner, overriding pytest's default, and then do the snapshot and resetting within it.

cc: @fubuloubu @unparalleled-js

Edit: there's also the option of fixtures, but must concur, snapshotting by default is most reasonable

Would pytest_sessionstart and pytest_sessionfinish work?

[skellet0r]

Would pytest_sessionstart and pytest_sessionfinish work?

Hmm, those hooks seem to be too early. I think maybe using pytest_collection_modifyitems may be what we're looking for? Along with some internal fixtures (_module_isolation etc), we can append them to tests like so: https://stackoverflow.com/a/50607635.

That and I guess adding a command line option --disable-isolation to turn off the default isolation would be just right.

The end result is, built-in isolation that users don't even have to worry about requesting, + an option to disable it if needed (for users that may want to roll their own or whatever). And since the fixture names start with an underscore, they're not shown in the pytest --fixtures output unless the -v option is included.

[skellet0r]

Ended up going for internal fixtures + modifying the setup phase to reorder the execution order of the fixtures.
End result is built-in isolation (no users fumbling around) + users can opt out with the --disable-isolation flag.
Isolation is done for all pytest scope levels (including class and package).

[antazoey]

I like the best of both worlds approach. Generally, I prefer truthy-flags (e.g. --enable-isolation) though I think this makes sense in this case.

[antazoey]

Does this need to be called "_session_isolation" to work? (Looking at the logic in the runners for dynamically setting the fixture)

[skellet0r]

ah the leading underscore you mean? That's just so it's considered internal, and not shown to the end user via pytest --fixtures

[fubuloubu]

I think this is a really interesting approach. This approval is for the approach, I still would like to try it locally and address @unparalleled-js's comments before merging.

[antazoey]

Thank you, this is awesome

[fubuloubu]

At first I thought this said "warn for unimplemented snapshot", would flipping the parity of this make it easier to read?

[skellet0r]

i've no clue lol, whatever seems best I'll rename it as (i've got no attachment), this name I just copy and pasted from the runner.py file

[fubuloubu]

Tested with https://github.com/yearn/veyfi (w/ upgrading most everything to session fixtures), and got this error:

$ ape test
=========================================================================================== test session starts ============================================================================================
platform linux -- Python 3.9.9, pytest-6.2.5, py-1.11.0, pluggy-0.13.1
rootdir: Yearn/v3/veYFI
plugins: eth-ape-0.1.5.dev33+g37efd453, web3-5.28.0
collected 45 items
INFO: Starting 'Hardhat node' process.

tests/functional/test_extra_rewards.py ......                                                                                                                                                        [ 13%]
tests/functional/test_gauge.py .........                                                                                                                                                             [ 33%]
tests/functional/test_gauge_reward_distribution.py .........                                                                                                                                         [ 53%]
tests/functional/test_ve_yfi_rewards.py .......                                                                                                                                                      [ 68%]
tests/functional/test_vote_delegation.py ...E                                                                                                                                                        [ 77%]
tests/functional/test_voting_escrow.py ..........                                                                                                                                                    [100%]
INFO: Stopping 'Hardhat node' process.


================================================================================================== ERRORS ==================================================================================================
___________________________________________________________________________ ERROR at setup of test_many_delegates_gas_usage[100] ___________________________________________________________________________

.0 = <list_iterator object at 0x7f069b209df0>

>   scopes = [item._fixtureinfo.name2fixturedefs[f][0].scope for f in item.fixturenames]
E   KeyError: '_session_isolation'

site-packages/ape/pytest/runners.py:96: KeyError
========================================================================================= short test summary info ==========================================================================================
ERROR tests/functional/test_vote_delegation.py::test_many_delegates_gas_usage[100] - KeyError: '_session_isolation'

[skellet0r]

Tested with https://github.com/yearn/veyfi (w/ upgrading most everything to session fixtures), and got this error:

ahh that was a nice catch, fixed in 4c9f87e