bpo-36954: Skip test_recursive_repr test when running test_xml_etree under coverage trace by GPHemsley · Pull Request #13267

bpo-36954: Skip test_recursive_repr test when running test_xml_etree under coverage trace by GPHemsley · Pull Request #13267 · python/cpython

…age trace

The test_recursive_repr test modifies the tracing environment, causing the
remainder of the tests to lose coverage information.

How does it modify the tracing environment?

Please open an issue for discussion. This is not trivial PR like fixing a typo.

The RuntimeError causes the settrace to get lost.

https://bugs.python.org/issue10933
https://bugs.python.org/issue23012
https://bugs.python.org/issue36474

Instead of skipping the test it should restore any sys.gettrace() in the end, e.g. for pytest it would be:

@pytest.fixture(autouse=True)
def restore_settrace():
    """(Re)store sys.gettrace after test run.

    This is required to re-enable coverage tracking.
    """
    assert sys.gettrace() is _orig_trace

    yield

    newtrace = sys.gettrace()
    if newtrace is not _orig_trace:
        sys.settrace(_orig_trace)
        assert newtrace is None

(I am using this with pdbpp's tests, where sys.settrace is called all the time in the tests)

..but since this is cpython itself, it would make more sense to fix the issue in the first place.. :)

To be clear, my goal with this PR was to stop this test from destroying the code coverage for the rest of the file. I didn't dive into why this test was causing the problem.

@serhiy-storchaka: Given the above tickets and discussion, do you still want a separate ticket for investigating this test?

To be clear, my goal with this PR was to stop this test from destroying the code coverage for the rest of the file.

Do you agree that it's better to restore tracing than not running the test at all?
(if all CI jobs use coverage, it would be skipped then completely)

To be clear, my goal with this PR was to stop this test from destroying the code coverage for the rest of the file.

Do you agree that it's better to restore tracing than not running the test at all?
(if all CI jobs use coverage, it would be skipped then completely)

The primary jobs on Travis CI (I can't speak to the others) do not use coverage; coverage is run in separate jobs. So the primary jobs will still run the test -- and will fail if the test fails.

Please open a new issue for coverage (or tracing?) and RecursionError. test_recursive_repr does not have particular relation to tracing, it is just one place where this issue is reproduced. I think you can create simple example that demonstrates the problem.

@serhiy-storchaka

Please open a new issue for coverage (or tracing?) and RecursionError

We have those already (see above).

test_recursive_repr does not have particular relation to tracing, it is just one place where this issue is reproduced. I think you can create simple example that demonstrates the problem.

It is not meant to show the issue, but this PR is about not losing coverage after this test (running into / affected by the issue) is being run.

@GPHemsley
I'd still suggest to restore tracing after the test (just wrap it in try / finally).

		e.extend([ET.Element('bar')])
		self.assertRaises(ValueError, e.remove, X('baz'))

		@unittest.skipIf(sys.gettrace(), "Skips under coverage.")

Choose a reason for hiding this comment

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

Just for completeness: this would also skip it when using pdb then.. ;)

Choose a reason for hiding this comment

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

For the record, I merely copied a line already used by another test. :)

Choose a reason for hiding this comment

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

GPHemsley changed the title ~~Skip test_recursive_repr test when running test_xml_etree under coverage trace~~ bpo-36954: Skip test_recursive_repr test when running test_xml_etree under coverage trace

May 18, 2019

This PR is stale because it has been open for 30 days with no activity.