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

Report correct outcome in the replay file (#64) #65

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

DavideCanton
Copy link
Contributor

Fixes #64

Added both tests for sequential and parallel usages.

Copy link
Member

@prusse-martin prusse-martin left a comment

Choose a reason for hiding this comment

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

Thank you.
That really did go unnoticed =)

Just one thing. When the "main" test pass but there is an error in a fixture teardown the recorded outcome will be "passed".

Comment on lines 93 to 95
if result.when == "call":
self.node_outcome[item.nodeid] = result.outcome
elif result.when == "teardown":
Copy link
Member

@prusse-martin prusse-martin Aug 26, 2024

Choose a reason for hiding this comment

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

Suggested change
if result.when == "call":
self.node_outcome[item.nodeid] = result.outcome
elif result.when == "teardown":
self.node_outcome.setdefault(item.nodeid, result.outcome)
if result.failed:
self.node_outcome[item.nodeid] = result.outcome
if result.when == "teardown":

This way all phases' outcomes are considered, otherwise a test could be flagged as "passed" even when a fixture teardown failed.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if result.when == "call":
self.node_outcome[item.nodeid] = result.outcome
elif result.when == "teardown":
self.node_outcome.setdefault(item.nodeid, result.outcome)
if not result.passed:
self.node_outcome[item.nodeid] = result.outcome
if result.when == "teardown":

BTW, "if not passed" is probably better. Given there is 3 possible outcomes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would not account for this case though:

import pytest

@pytest.fixture()
def foo():
    yield
    pytest.skip("skipping")

def test(foo):
    assert False

This would overwrite "failed" with "skipped", while I guess the test should be considered failed.

Comment on lines 278 to 298
testdir.makepyfile(
test_module="""
def test_success():
pass

def test_failure():
assert False
"""
)
dir = testdir.tmpdir / "replay"
result = testdir.runpytest_subprocess(f"--replay-record-dir={dir}")
assert result.ret != 0

contents = [json.loads(s) for s in (dir / ".pytest-replay.txt").read().splitlines()]
assert len(contents) == 4

assert "test_success" in contents[1]["nodeid"]
assert contents[1]["outcome"] == "passed"

assert "test_failure" in contents[3]["nodeid"]
assert contents[3]["outcome"] == "failed"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
testdir.makepyfile(
test_module="""
def test_success():
pass
def test_failure():
assert False
"""
)
dir = testdir.tmpdir / "replay"
result = testdir.runpytest_subprocess(f"--replay-record-dir={dir}")
assert result.ret != 0
contents = [json.loads(s) for s in (dir / ".pytest-replay.txt").read().splitlines()]
assert len(contents) == 4
assert "test_success" in contents[1]["nodeid"]
assert contents[1]["outcome"] == "passed"
assert "test_failure" in contents[3]["nodeid"]
assert contents[3]["outcome"] == "failed"
testdir.makepyfile(
test_module="""
import pytest
def test_success():
pass
def test_failure():
assert False
@pytest.fixture
def some_fixture():
yield
assert False
def test_failure_fixture_teardown(some_fixture):
assert True
"""
)
dir = testdir.tmpdir / "replay"
result = testdir.runpytest_subprocess(f"--replay-record-dir={dir}")
assert result.ret != 0
contents = [json.loads(s) for s in (dir / ".pytest-replay.txt").read().splitlines()]
outcomes = {r["nodeid"]: r["outcome"] for r in contents if "outcome" in r}
assert outcomes == {
"test_module.py::test_success": "passed",
"test_module.py::test_failure": "failed",
"test_module.py::test_failure_fixture_teardown": "failed",
}

Updating to test my previous suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this misses a test for a setup failure, adding it

@DavideCanton
Copy link
Contributor Author

DavideCanton commented Aug 27, 2024

Just one thing. When the "main" test pass but there is an error in a fixture teardown the recorded outcome will be "passed".

I kinda assumed the user is mainly interested in the actual test outcome, but probably your guess is better.

@DavideCanton
Copy link
Contributor Author

@prusse-martin I've updated both code and tests to consider also the setup phase and to give priority to failed outcomes over the skipped ones. Waiting for you review!

@prusse-martin
Copy link
Member

Thank you for fixing this.

@prusse-martin prusse-martin merged commit 0ae6bc8 into ESSS:master Aug 28, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong outcome when test fails
2 participants