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.

Sign up for GitHub

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

Jump to bottom

Fix tests reported as failures when rerun happened due to exception raised from fixture teardown when using only_rerun #262

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix tests reported as failures when rerun happened due to exception raised from fixture teardown when using only_rerun #262

wants to merge 6 commits into from

Conversation

shlompy
Copy link

@shlompy shlompy commented Feb 25, 2024 •
edited by icemac
Loading

Fixes #261

@icemac
Copy link
Contributor

icemac commented Feb 26, 2024

@shlompy Thank you for your PR. Currently many tests break. Could you please have a look what's causing this?

@shlompy
Copy link
Author

shlompy commented Feb 26, 2024

Thanks @icemac
I hope I fixed the precommit issues.
As for test_execution_count_exposed test failures, I changed the assert. It looks like the test assertion was wrong due to the bug.

I hope I have not misunderstood.
This test should fail in teardown until teardown will succeed on the third run - Meaning it should have had 2 reruns and 1 pass.
I'm not sure why the test asserted for 3 passes and 2 reruns in first place. Probably it took into consideration the bug, because the teardown failed twice and passed one time, but when it failed twice, it was too late because the 'call' (test) report was already concluded as pass.

Now with my bug fix the outcome is indeed 1 pass and 2 reruns, so I've updated the test assertion to match that.

But I don't see the test git actions being triggered again after my last push

@icemac icemac requested a review from hugovk February 28, 2024 07:43
@hugovk
Copy link
Member

hugovk commented Feb 28, 2024

Thanks for the review request, but I'll stick to general CI/maintenance things for this library and leave the internals to others 👍

@hugovk hugovk removed their request for review February 28, 2024 09:22
@shlompy
Copy link
Author

shlompy commented Mar 1, 2024

resolved conflicts

@shlompy
Copy link
Author

shlompy commented Mar 3, 2024

gentle reminder, can this fix please be reviewed?

icemac
icemac requested changes Mar 7, 2024
View reviewed changes
Copy link
Contributor

@icemac icemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear why the test breaks, it looks like a smell.

I'd like to see a test like you had in the issue failing without the changes and succeeding with them.

tests/test_pytest_rerunfailures.py
@@ -472,7 +472,7 @@ def pytest_runtest_teardown(item):
assert item.execution_count == 3"""
)
result = testdir.runpytest("--reruns", "2")
assert_outcomes(result, passed=3, rerun=2)
assert_outcomes(result, passed=1, rerun=2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name says that execution_count is exposed to the test runner. So it should be 3 passed tests, as this is set during tear down.

I do not know why this behaviour is here and why it breaks now.

src/pytest_rerunfailures.py Outdated Show resolved Hide resolved
@icemac icemac added the do not merge Not (yet) ready to be merged. label Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icemac icemac icemac requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
do not merge Not (yet) ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

only_rerun with exception raised from fixture teardown reruns test but report previous runs as failure
4 participants
@shlompy @icemac @hugovk @samitAtsyna