Fix teardown error#101
Conversation
|
@Dreamsorcerer could you credit the patch author via https://hynek.me/til/easier-crediting-contributors-github/ and add a change log entry (it's not managed by Towncrier and is updated manually)? I'm not confident in understanding this atm so I'll ask @RonnyPfannschmidt to review. |
There was a problem hiding this comment.
Pull request overview
Fixes the “previous item was not torn down properly” error seen on pytest 7.x by ensuring teardown is performed after running forked tests, and adds a regression test for the scenario described in #67.
Changes:
- Add a regression test covering a forked test followed by a test in a different module.
- Update
pytest_runtest_protocolto receivenextitemand explicitly call teardown after reporting forked results. - Bump runtime dependency to
pytest>=7.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| testing/test_boxed.py | Adds a regression test ensuring teardown happens when a forked test is followed by another module’s test. |
| src/pytest_forked/init.py | Calls teardown_exact(nextitem) after running forked tests to satisfy pytest 7’s teardown expectations. |
| setup.py | Raises the declared minimum supported pytest version to 7. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
Unfortunately this one is also a breaking change for anything that tears down external resources
Thr forked plugin is fundamentally broken
OK, so where do we go from here? I need a solution to be able to update and maintain aiohttp-devtools, and this patch seems like it gets things unblocked for us. Do we accept this breakage as an alternative to the current breakage? Or can we use something else for our tests? This library still seems to have a surprising amount of commits for something that is fundamentally broken. |
It's a breaking change, but do you think the testsuites that rely on the current behavior are "legitimate"? It seems to me the only way this could break things is if there's a testsuite depending on execution order or depending on module state always being torn down right before session state. I guess the easiest way forward is to add a flag to restore the old behavior for anybody who really needs it? |
|
A breaking release is probably fine as well This project is practically decommissioned and there's no way to reconciled multi scope setupstate and forked Im fine with letting people that still need it bending it however needed to keep their testsuites running |
|
I'm not familiar with pytest internals, but I asked Claude to make a suggestion, and it seems to think your concern would be solved by adding this line: def runforked():
+ item.session._setupstate.stack.clear()If you think that looks correct, I'll update it and add some extra tests. |
|
That looks like another one's of tjos classic claude failures When it comes to the internals of pytest and pluggy both sonnet and opus demonstrate utter failure in their plausible emulation if intellect |
|
The mobile githu app seems to point me to items in reverse order over time and it's annoying as the ux looks like im replying to a new note Github is slowly falling Apparat |
|
Alright, I'll leave the fix as is then. |
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
lets make this one a major release noting the need changes
Fixes #67.
Confirmed locally that it fixes the problem on aiohttp-devtools.
Co-authored-by: Barney Gale 960340+barneygale@users.noreply.github.com