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

Fixed tests that were failing due Pytest updates #190

Merged
merged 10 commits into from
Jan 11, 2019

Conversation

RibeiroAna
Copy link
Member

Comments explaining how I did it on the code.

@@ -12,6 +12,7 @@ dist
# IDE Specific files
### Pycharm IDE - Jetbrains
.idea/*
.vscode/*
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using Visual Studio right now, so I added it.

@@ -113,8 +113,6 @@ def __init__(self, outcome, report, logfile, config):
self.config = config
self.row_table = self.row_extra = None

test_index = hasattr(report, 'rerun') and report.rerun + 1 or 0
Copy link
Member Author

Choose a reason for hiding this comment

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

I realised from here that report doesn't have the attribute rerun anymore, so that was causing some rerun tests to fail. Instead of getting the test_index from the attribute, I'm passing it as a parameter from the property rerun of the TestResult class.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be a regression from pytest-rerunfailures. It would seem that the latest version doesn't report the number of reruns in the report? If so, please raise an issue against the plugin. In the meantime we could pin our tests to the version of pytest-rerunfailures before this change.

@@ -140,7 +140,9 @@ def test_xpass(): pass

def test_setup_error(self, testdir):
testdir.makepyfile("""
def pytest_funcarg__arg(request):
Copy link
Member Author

Choose a reason for hiding this comment

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

Funcarg is deprecated since Pytest 4.

@@ -442,7 +444,7 @@ def pytest_runtest_makereport(item, call):
""".format(extra_type, content))
testdir.makepyfile("""
import pytest
@pytest.mark.flaky(reruns=2)
@pytest.mark.flaky(reruns=4)
Copy link
Member Author

@RibeiroAna RibeiroAna Jan 4, 2019

Choose a reason for hiding this comment

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

Just because the loop bellow goes from 1 to 4 (so it checks for extra content 4 times) and there was only 2 reruns.

Copy link
Collaborator

@davehunt davehunt Jan 4, 2019

Choose a reason for hiding this comment

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

I see your logic, but I wonder why we had this mismatch with tests passing? This code hasn't changed in three years.

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw the logic there before, actually the loop checks for content 3 times (range(1,4) = [1, 2, 3], my fault) and the first test (the one which failed) count as the file1, the first rerun is actually file2, and so on

@RibeiroAna RibeiroAna changed the title Fixed tests that were failing because of Pytest updates Fixed tests that were failing due Pytest updates Jan 4, 2019
@@ -263,7 +261,8 @@ def append_log_html(self, report, additional_html):
additional_html.append(log)

def _appendrow(self, outcome, report):
result = self.TestResult(outcome, report, self.logfile, self.config)
result = self.TestResult(outcome, report, self.logfile, self.config,
self.rerun)
Copy link
Collaborator

Choose a reason for hiding this comment

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

self.rerun will only be present if the pytest-rerunfailures plugin is installed, if it's not installed I would expect this to raise an exception due to the attribute not being available

@@ -113,8 +113,6 @@ def __init__(self, outcome, report, logfile, config):
self.config = config
self.row_table = self.row_extra = None

test_index = hasattr(report, 'rerun') and report.rerun + 1 or 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be a regression from pytest-rerunfailures. It would seem that the latest version doesn't report the number of reruns in the report? If so, please raise an issue against the plugin. In the meantime we could pin our tests to the version of pytest-rerunfailures before this change.

@@ -442,7 +444,7 @@ def pytest_runtest_makereport(item, call):
""".format(extra_type, content))
testdir.makepyfile("""
import pytest
@pytest.mark.flaky(reruns=2)
@pytest.mark.flaky(reruns=4)
Copy link
Collaborator

@davehunt davehunt Jan 4, 2019

Choose a reason for hiding this comment

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

I see your logic, but I wonder why we had this mismatch with tests passing? This code hasn't changed in three years.

davehunt
davehunt previously approved these changes Jan 7, 2019
Copy link
Collaborator

@davehunt davehunt left a comment

Choose a reason for hiding this comment

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

Please squash before merging.

tox.ini Outdated
@@ -10,7 +10,7 @@ envlist = py{27,36,37,py,py3}{,-ansi2html}, flake8
commands = pytest -v -r a {posargs}
deps =
pytest-xdist
pytest-rerunfailures
pytest-rerunfailures <= 4.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment referring to pytest-dev/pytest-rerunfailures#77 so we can remove this in the future. Otherwise, this lgtm.

@RibeiroAna
Copy link
Member Author

Hi @davehunt Pytest-rerunfailures 6.0 has been released with my fix. Could you please approve this PR so we can have pytest-html with tests passing once again?

@davehunt
Copy link
Collaborator

Sorry for the delay, and thanks @RibeiroAna!

@davehunt davehunt merged commit 6f9b14c into pytest-dev:master Jan 11, 2019
@ssbarnea ssbarnea added the bug This issue/PR relates to a bug. label Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants