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

Allow tests to be logged multiple times with pytest-xdist #1193

Closed
davehunt opened this Issue Nov 25, 2015 · 8 comments

Comments

Projects
None yet
4 participants
@davehunt
Contributor

davehunt commented Nov 25, 2015

Note that this is not related to #927

In pytest-rerunfailures the test protocol is run multiple times in the event of a failure. An enhancement will log each iteration of the test with an outcome of 'rerun' until the maximum number of reruns is reached, at which point the genuine outcome is preserved. This works fine unless running the tests without pytest-xdist, which causes issues when the first rerun of a test is attempted to be removed from the scheduler:

INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/Users/dhunt/.virtualenvs/tmp-aae67d87c3b61cd1/lib/python2.7/site-packages/pytest-2.8.3-py2.7.egg/_pytest/main.py", line 90, in wrap_session
INTERNALERROR>     session.exitstatus = doit(config, session) or 0
INTERNALERROR>   File "/Users/dhunt/.virtualenvs/tmp-aae67d87c3b61cd1/lib/python2.7/site-packages/pytest-2.8.3-py2.7.egg/_pytest/main.py", line 121, in _main
INTERNALERROR>     config.hook.pytest_runtestloop(session=session)
INTERNALERROR>   File "/Users/dhunt/.virtualenvs/tmp-aae67d87c3b61cd1/lib/python2.7/site-packages/pytest-2.8.3-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 724, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File "/Users/dhunt/.virtualenvs/tmp-aae67d87c3b61cd1/lib/python2.7/site-packages/pytest-2.8.3-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 338, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/Users/dhunt/.virtualenvs/tmp-aae67d87c3b61cd1/lib/python2.7/site-packages/pytest-2.8.3-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 333, in <lambda>
INTERNALERROR>     _MultiCall(methods, kwargs, hook.spec_opts).execute()
INTERNALERROR>   File "/Users/dhunt/.virtualenvs/tmp-aae67d87c3b61cd1/lib/python2.7/site-packages/pytest-2.8.3-py2.7.egg/_pytest/vendored_packages/pluggy.py", line 596, in execute
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/Users/dhunt/.virtualenvs/tmp-aae67d87c3b61cd1/lib/python2.7/site-packages/xdist/dsession.py", line 521, in pytest_runtestloop
INTERNALERROR>     self.loop_once()
INTERNALERROR>   File "/Users/dhunt/.virtualenvs/tmp-aae67d87c3b61cd1/lib/python2.7/site-packages/xdist/dsession.py", line 539, in loop_once
INTERNALERROR>     call(**kwargs)
INTERNALERROR>   File "/Users/dhunt/.virtualenvs/tmp-aae67d87c3b61cd1/lib/python2.7/site-packages/xdist/dsession.py", line 643, in slave_testreport
INTERNALERROR>     self.sched.remove_item(node, rep.item_index, rep.duration)
INTERNALERROR>   File "/Users/dhunt/.virtualenvs/tmp-aae67d87c3b61cd1/lib/python2.7/site-packages/xdist/dsession.py", line 280, in remove_item
INTERNALERROR>     self.node2pending[node].remove(item_index)
INTERNALERROR> ValueError: list.remove(x): x not in list

For now I can avoid logging multiple reports if we're running tests in parallel, but that does mean that the only indication of multiple tests running will be the multiple logstart calls. Ideally there should be no difference between running tests in serial or parallel.

@nicoddemus

This comment has been minimized.

Show comment
Hide comment
@nicoddemus

nicoddemus Nov 25, 2015

Member

Hmmm I'm not sure how to implement that other than making xdist explicitly know about "reruns".

    def slave_testreport(self, node, rep):
        if rep.when == "call" or (rep.when == "setup" and not rep.passed):
            self.sched.remove_item(node, rep.item_index, rep.duration)

xdist assumes that a "call" report is enough to remove the item from the scheduler, but that should not be the case when report.outcome == "rerun" as the test will be executed again.

I'm not sure how to solve this without adding new hooks to xdist, for example a hook pytest_xdist_test_finished(node, report) which allows plugins to override the check to know when a slave is done with a test based on the test report.

Another idea would be to grow a new attribute into the report object, something like report.done or report.finished... then xdist can use that instead of rep.when == "call" or (rep.when == "setup" and not rep.passed), and pytest-rerunfailures can set that attribute to False to make it clear the item is not finished yet.

IMHO though I don't see a problem with rerun behaving slightly differently when running under xdist, specially when it comes to reporting only.

Member

nicoddemus commented Nov 25, 2015

Hmmm I'm not sure how to implement that other than making xdist explicitly know about "reruns".

    def slave_testreport(self, node, rep):
        if rep.when == "call" or (rep.when == "setup" and not rep.passed):
            self.sched.remove_item(node, rep.item_index, rep.duration)

xdist assumes that a "call" report is enough to remove the item from the scheduler, but that should not be the case when report.outcome == "rerun" as the test will be executed again.

I'm not sure how to solve this without adding new hooks to xdist, for example a hook pytest_xdist_test_finished(node, report) which allows plugins to override the check to know when a slave is done with a test based on the test report.

Another idea would be to grow a new attribute into the report object, something like report.done or report.finished... then xdist can use that instead of rep.when == "call" or (rep.when == "setup" and not rep.passed), and pytest-rerunfailures can set that attribute to False to make it clear the item is not finished yet.

IMHO though I don't see a problem with rerun behaving slightly differently when running under xdist, specially when it comes to reporting only.

@Dude-X

This comment has been minimized.

Show comment
Hide comment
@Dude-X

Dude-X Feb 9, 2016

Related: This should affect Box's flaky plugin for pytest.

Dude-X commented Feb 9, 2016

Related: This should affect Box's flaky plugin for pytest.

@RonnyPfannschmidt

This comment has been minimized.

Show comment
Hide comment
@RonnyPfannschmidt

RonnyPfannschmidt Jan 18, 2017

Member

this issue cant be fixed unless we reassert that test node ids are unique

Member

RonnyPfannschmidt commented Jan 18, 2017

this issue cant be fixed unless we reassert that test node ids are unique

@davehunt

This comment has been minimized.

Show comment
Hide comment
@davehunt

davehunt Jul 20, 2017

Contributor

@nicoddemus & @RonnyPfannschmidt do you have any new thoughts on this? I'm keen to address it, but may need some assistance from one or both of you.

Contributor

davehunt commented Jul 20, 2017

@nicoddemus & @RonnyPfannschmidt do you have any new thoughts on this? I'm keen to address it, but may need some assistance from one or both of you.

@RonnyPfannschmidt

This comment has been minimized.

Show comment
Hide comment
@RonnyPfannschmidt

RonnyPfannschmidt Jul 20, 2017

Member

this issue is sill unfixable without changing xdist at its core
but we should move it to xdist

Member

RonnyPfannschmidt commented Jul 20, 2017

this issue is sill unfixable without changing xdist at its core
but we should move it to xdist

@davehunt

This comment has been minimized.

Show comment
Hide comment
@davehunt

davehunt Aug 1, 2017

Contributor

@RonnyPfannschmidt can we move the issue via some admin interface, or would you like me to raise it under xdist and close this one?

Contributor

davehunt commented Aug 1, 2017

@RonnyPfannschmidt can we move the issue via some admin interface, or would you like me to raise it under xdist and close this one?

@RonnyPfannschmidt

This comment has been minimized.

Show comment
Hide comment
@RonnyPfannschmidt

RonnyPfannschmidt Aug 1, 2017

Member

there is no admin ui to do that, gh is just fancy, not cappable

Member

RonnyPfannschmidt commented Aug 1, 2017

there is no admin ui to do that, gh is just fancy, not cappable

@davehunt

This comment has been minimized.

Show comment
Hide comment
@davehunt

davehunt Aug 7, 2017

Contributor

Kamino closed and cloned this issue to pytest-dev/pytest-xdist#206

Contributor

davehunt commented Aug 7, 2017

Kamino closed and cloned this issue to pytest-dev/pytest-xdist#206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment