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

fix: update isolation as new fixtures of certain scopes arrive #2277

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

antazoey
Copy link
Member

@antazoey antazoey commented Sep 11, 2024

What I did

Make isolation "work" on session, package, module, and class scopes.
Basically, the bug was that as new fixtures of certain scopes come in after a snapshot was taken in that scope, they wouldn't really persist as the scope below left, like if the module changes or something

fixes: #2262
fixes: #1287

How I did it

Awful. Basically the only way to fix this is to rebase the blockchain as new upper scopes up.
A new session comes in later, we have to undo to the module (or closest lowest) snapshot invalidate those fixture, inject those fixtures in the correct order, and re-set all the snapshots.

How to verify it

Can technically uses scopes / parametrized fixtures in any way now... and it should also "work"... it is just is slow if using higher scoped fixtures later on in the test suite, can be really bad actually. I definitely don't recommend using parametrized fixtures unless I can figure out a more optimal way with those

Checklist

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

fubuloubu
fubuloubu previously approved these changes Sep 11, 2024
@antazoey antazoey changed the title fix: avoid restore if snapshot changed supports boolean fix: issue with snapshot ID of 0 and avoid restore during failure Sep 11, 2024
@antazoey antazoey changed the title fix: issue with snapshot ID of 0 and avoid restore during failure fix: various issues related to snapshotting and test-isolation Sep 11, 2024
@antazoey antazoey marked this pull request as draft September 11, 2024 17:17
@antazoey
Copy link
Member Author

suspecting something with the runner now...

src/ape/pytest/runners.py Outdated Show resolved Hide resolved
@antazoey
Copy link
Member Author

antazoey commented Sep 11, 2024

  • Ensure isolation fixtures ran last (this has to be the problem!)

@antazoey antazoey changed the title fix: various issues related to snapshotting and test-isolation fix: isolation fixtures were running out of order Sep 11, 2024
@antazoey
Copy link
Member Author

Update, mentioned on Telegram but I am pretty sure I actually found the bug; I am still working on the full fix this PR is not ready for testing yet). Standby for updates.

fubuloubu
fubuloubu previously approved these changes Sep 11, 2024
@antazoey
Copy link
Member Author

If you want to see the most magical part(s), look at the changes made to the runner.py module.

@antazoey
Copy link
Member Author

antazoey commented Sep 13, 2024

This is "working" but may have performance issues if users don't use autouse=True when they should or use parametrized session-scoped fixtures.

src/ape/pytest/utils.py Outdated Show resolved Hide resolved
@antazoey antazoey changed the title fix: isolation fixtures were running out of order fix: update isolation as new fixtures of certain scopes arrive Sep 13, 2024
@antazoey
Copy link
Member Author

antazoey commented Sep 13, 2024

  • Either do or consider only reinitializing the autouse=True fixtures (rest should still be invalidated but maybe dont replay them
    What I did: no longer auto reinclude anything. autouse=True fixtures will run again anyway, and the workaround for if you need a fixture to for sure exist, make sure it is used somewhere, even if by another fixture. it actually ain't that hard.

  • Ignore fixtures that don't change chain head from invalidation consideration
    Questions: How does one know if a fixture has caused the chain height to increase? There is a way to know if an entire has or if a test has, but there doesn't seem to be a way to know if an individual fixture has, that I can think of.

  • For parametrized fixtures, (attempt) to delay the invalidation until the last iteration runs (this will make using them acceptable imo)
    Notes: This seems to work and makes parametrized fixtures ok to use in my book, so yay.

docs/userguides/testing.md Outdated Show resolved Hide resolved
docs/userguides/testing.md Outdated Show resolved Hide resolved
docs/userguides/testing.md Outdated Show resolved Hide resolved
@fubuloubu
Copy link
Member

Questions: How does one know if a fixture has caused the chain height to increase? There is a way to know if an entire has or if a test has, but there doesn't seem to be a way to know if an individual fixture has, that I can think of.

Can we store extra info in the cache? Such as block height of last access? If auto-mining is used, that would roughly correspond to 1 block = 1 transaction, so this is a good approximation (If auto-mining is turned off, this would not work)

@antazoey
Copy link
Member Author

antazoey commented Sep 18, 2024

Can we store extra info in the cache? Such as block height of last access? If auto-mining is used, that would roughly correspond to 1 block = 1 transaction, so this is a good approximation (If auto-mining is turned off, this would not work)

Yes, this is how we would do it. I just can't figure out how to know which fixture is responsible. It seems really complicated. For example, if you were using multiple module-scoped fixtures at the same time and some caused height increase and some didn't, the chain height would be higher the next usage for all of them, so how does one know which fixture caused the bump?

Edit: Going to see if there a hook for when fixtures run, that may work... liike pytest_fixture_post_finalizer

@antazoey
Copy link
Member Author

antazoey commented Sep 18, 2024

  • fix parametrized test from apepay
    seems related to parametrized tests, maybe the param treated as a fixture or something

Fixed by 4beeaf3

@antazoey
Copy link
Member Author

antazoey commented Sep 18, 2024

  • Fix create_stream min time goes to zero issues that has arrived in ApePay

@antazoey
Copy link
Member Author

antazoey commented Sep 18, 2024

  • Fix create_stream min time goes to zero issues that has arrived in ApePay

Notes:

  • Figured out there is a problem when using multiple parametrized fixtures at once.

@antazoey
Copy link
Member Author

antazoey commented Sep 30, 2024

  • Only trigger invalidation on stateful fixtures of higher scope
  • Better handling of parametrized fixtures - right now, always treats them as "new", meaning it is wayy overly invalidating...

fubuloubu
fubuloubu previously approved these changes Oct 1, 2024
fubuloubu
fubuloubu previously approved these changes Oct 14, 2024
@antazoey
Copy link
Member Author

So before I merge this, I am going to add a few more projects to the integration tester thing.
I want to really make sure I am not wrecking all of tests by doing this.

@antazoey
Copy link
Member Author

ApePay

this branch
32 passed in 102.98s (0:01:42)

main
32 passed in 106.16s (0:01:46)

@antazoey antazoey mentioned this pull request Oct 21, 2024
3 tasks
@antazoey
Copy link
Member Author

Once I get #2327 to where I like, I want to also compare running Ape's tests w/ and w/o this feature.

@antazoey antazoey force-pushed the fix/isolation-issue branch 2 times, most recently from 40e1e69 to 39ec0dc Compare November 5, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants