pytest: handle non-zero exit code

[blueyed]

TODO:

• TestRunnerFailure should be made available for test runners in general?! Should it get derived from some CosmicRayException base? Should there be a exceptions module for this?
• in a test only io.StringIO is used (without the compat). Should be done here then also I assume?

[blueyed]

Without this PR I get invalid mutants, since the pytest failure is not detected.

With this PR it looks like this then:

{
  "data": [
    "Traceback (most recent call last):\n",
    "  File \"…/Vcs/cosmic-ray/cosmic_ray/testing/test_runner.py\", line 56, in __call__\n    test_result = self._run()\n",
    "  File \"…/Vcs/cosmic-ray/plugins/test-runners/pytest/cosmic_ray_pytest_runner/runner.py\", line 54, in _run\n    raise TestRunnerFailure('pytest exited non-zero', exit_code, output)\n",
    "cosmic_ray_pytest_runner.runner.PytestRunner._run.<locals>.TestRunnerFailure: ('pytest exited non-zero', 2, \"============================= test session starts ==============================\\nplatform linux -- Python 3.6.3, pytest-3.2.3, py-1.4.34, pluggy-0.4.0\\nrootdir: …/src/covimerage, inifile: setup.cfg\\nplugins: pdb-0.2.0, mock-1.6.3, cov-2.5.1, catchlog-1.2.2\\ncollected 10 items / 3 errors\\n\\n=========================== short test summary info ============================\\nERROR tests/test_cli.py\\nERROR tests/test_main.py\\nERROR tests/test_utils.py\\n==================================== ERRORS ====================================\\n______________________ ERROR collecting tests/test_cli.py ______________________\\nImportError while importing test module '…/src/covimerage/tests/test_cli.py'.\\nHint: make sure your test modules/packages have valid Python names.\\nTraceback:\\ntests/test_cli.py:11: in <module>\\n    from covimerage import DEFAULT_COVERAGE_DATA_FILE, cli\\n../../Vcs/cosmic-ray/cosmic_ray/importing.py:33: in exec_module\\n    exec(compiled, mod.__dict__)  # pylint:disable=exec-used\\ncovimerage:10: in <module>\\n    ???\\nE   ImportError: attempted relative import with no known parent package\\n_____________________ ERROR collecting tests/test_main.py ______________________\\nImportError while importing test module '…/src/covimerage/tests/test_main.py'.\\nHint: make sure your test modules/packages have valid Python names.\\nTraceback:\\ntests/test_main.py:6: in <module>\\n    from covimerage._compat import FileNotFoundError, StringIO\\n../../Vcs/cosmic-ray/cosmic_ray/importing.py:33: in exec_module\\n    exec(compiled, mod.__dict__)  # pylint:disable=exec-used\\ncovimerage:10: in <module>\\n    ???\\nE   ImportError: attempted relative import with no known parent package\\n_____________________ ERROR collecting tests/test_utils.py _____________________\\nImportError while importing test module '…/src/covimerage/tests/test_utils.py'.\\nHint: make sure your test modules/packages have valid Python names.\\nTraceback:\\ntests/test_utils.py:1: in <module>\\n    from covimerage._compat import StringIO\\n../../Vcs/cosmic-ray/cosmic_ray/importing.py:33: in exec_module\\n    exec(compiled, mod.__dict__)  # pylint:disable=exec-used\\ncovimerage:10: in <module>\\n    ???\\nE   ImportError: attempted relative import with no known parent package\\n!!!!!!!!!!!!!!!!!!! Interrupted: 3 errors during collection !!!!!!!!!!!!!!!!!!!!\\n=========================== 3 error in 0.27 seconds ============================\\n\")\n"
  ],
  "test_outcome": "incompetent",
  "worker_outcome": "normal",
  "diff": [
    "--- mutation diff ---",
    "--- a…/src/covimerage/covimerage/__init__.py",
    "+++ b…/src/covimerage/covimerage/__init__.py",
    "@@ -293,7 +293,7 @@",
    "                 if next_line.startswith('count'):",
    "                     break",
    "             return skipped",
    "-        for line in file_object:",
    "+        for line in []:",
    "             plnum += 1",
    "             line = line.rstrip('\\r\\n')",
    "             if (line == ''):"
  ],
  "module": null,
  "operator": "cosmic_ray.operators.zero_iteration_loop.ZeroIterationLoop",
  "occurrence": 9,
  "line_number": 357,
  "command_line": null,
  "job_id": null
}

I think TestRunnerFailure should be handled in a special way then, since e.g. the traceback makes not much sense for it, but its exit_code and output should have entries in the JSON then?!

[blueyed]

pytest supports these exit codes:

Exit code 0: All tests were collected and passed successfully
Exit code 1: Tests were collected and run but some of the tests failed
Exit code 2: Test execution was interrupted by the user
Exit code 3: Internal error happened while executing tests
Exit code 4: pytest command line usage error
Exit code 5: No tests were collected

Related: pytest-dev/pytest#2950

[rob-smallshire]

Cosmic Ray is Python >= 3.4 so from io import StringIO is fine with no need for the Python 2 compatibility.

[rob-smallshire]

No need for this. Just use from io import StringIO

[rob-smallshire]

The StringIO object is never close()d – this isn't super important, but it's good practice rather than relying on the garbage collect to dispose of the buffer. We should probably manage so stdout object using its context-manager interface, so:

with StringIO() as stdout:

[blueyed]

Thanks for the review already.

What do you think about having TestRunnerFailure, possibly in an expceptions module?

[rob-smallshire]

I'm not much in favour of centralising all the exception definitions. I'd rather have them organised with the code they are relevant to. A better place for TestRunnerFailure could be cosmic_ray/testing/test_runner.py, which could also enable the except Exception clause in the __call__() method to be narrowed and the expected exception to be documented as part of the _run() API. All said though, I believe we should wait for @abingham's input, as he's much more familiar with this than me.

[abingham]

Can this import be at the top of the module? That would be more idiomatic, less surprising, etc.

[blueyed]

Sure, will do it for now (but only later) - in the long run this should go/come with TestRunner likely anyway.

[abingham]

All in all this looks like a good change. It's certainly more thoroughly thought-out than my initial implementation.

I'm still going around in circles trying to decide if a special TestRunnerFailure exception is useful. The main question I have is whether we'd really treat it any differently from any other exception that might escape from TestRunner._run() implementations. CR has to be robust against all forms of exceptions coming from test runners, all up and down the chain of invocations. If we don't plan on doing anything special with TestRunnerFailure, then the extra code seems like more of a liability than a benefit. Then again, "explicit is better than implicit", so there's a philosophical argument for it.

I'm happy to be convinced one way or the other. Perhaps I'm missing some important point here!

[blueyed]

I think the benefit from having TestRunnerFailure would be the interface it provides, e.g. exit_code and output, that can be displayed better than the raw exception in a list of lines.
It is also a way for the runner to signal something (i.e. we might have subclasses based on it later), distinguishing it from "any/some exception".

[blueyed]

Can you explain the test failure? (haven't investigated - is it because of incompetent vs. failed?!)

____________________________ test_e2e[pytest-local] ____________________________
project_root = PosixPath('/home/travis/build/sixty-north/cosmic-ray/test/test_project')
test_runner = 'pytest', engine = 'local'
    def test_e2e(project_root, test_runner, engine):
        config = 'cosmic-ray.{}.{}.conf'.format(test_runner, engine)
        session = 'adam-tests.{}.{}.session.json'.format(test_runner, engine)
    
        subprocess.check_call(['cosmic-ray', 'init', config, session],
                              cwd=str(project_root))
        subprocess.check_call(['cosmic-ray', 'exec', session],
                              cwd=str(project_root))
    
        session_path = project_root / session
        with use_db(str(session_path), WorkDB.Mode.open) as work_db:
            rate = survival_rate(work_db.work_items)
>           assert rate == 0.0
E           assert 98.46153846153847 == 0.0

[abingham]

Can you explain the test failure?

No explanation right now. I don't think it can be incompetent mutants; those count as killed for reporting purposes. What we have here is a very high survival rate, so somehow the test suites are passing.

If I do the mutation run directly on my machine, everything works as it should (i.e. no survivors). However, if I run tox then I see the survivors. So it's something to do with tox or its configuration, I think.

[rob-smallshire]

I like the TestRunnerFailure approach. Rather than TestRunner needing to handle any, arbitrary Exception (reminder than this includes AssertionError, IndentationError, SyntaxError and whatnot which might occur in a TestRunner subclass during development), we have a domain-specific exception type TestRunnerFailure and it's up to concrete TestRunners to convert whatever they receive via exit codes, exceptions, stdout messages, or whatever, into that type.

[blueyed]

I am seeing the same failure with pytest -x test/e2e/test_e2e.py (after having it rebased at least).

[abingham]

The problem is in the report generator. It's not accounting for incompetent mutants, i.e. WorkItem.test_outcome == TestOutcome.INCOMPETENT. This is just a bug.

I think you can fix this properly by changing cosmic_ray/reporting.py:45 to:

        if record.test_outcome in {TestOutcome.KILLED, TestOutcome.INCOMPETENT}:

or something equivalent.

If you want to make this change as part of your PR, I'll be fine with that. Or if you'd prefer I can make a separate change and you can merge it in.

In either case, it sounds like we need an automated test for this situation.

[blueyed]

re the test failure: the exit code is 2 since -x is used and that raises the TestRunnerFailure, resulting in "incompetent" outcome, and those are not considered to be "killed" (which is good I think ?! - apparently not to your previous comment) - see

cosmic-ray/cosmic_ray/reporting.py

Lines 39 to 47 in d45aac7

	def is_killed(record):
	"""Determines if a WorkItem should be considered "killed".
	"""
	if record.worker_outcome == WorkerOutcome.TIMEOUT:
	return True
	elif record.worker_outcome == WorkerOutcome.NORMAL:
	if record.test_outcome == TestOutcome.KILLED:
	return True
	return False

.

[blueyed]

Created #325 for now, let's see if it is covered already.

[abingham]

those are not considered to be "killed" (which is good I think ?! - apparently not to your previous comment)

At least for the purposes of reporting, incompetent mutants are considered killed. The idea is that, since they couldn't even run, the test suite is not passing the mutants. This is as good as killed for the purposes of gauging test suite quality.

[abingham]

@blueyed With the exception of the linting error on travis, do you consider this ready to merge? If so, I'll merge it in and fix the lint issue separately.

I figure we can deal with the TestRunnerFailure generalization in subsequent changes.

[blueyed]

@abingham
I wanted to look into looking at r.when additionally (https://github.com/pytest-dev/pytest/blob/d5f038e29ab3882eed8779efc98fbc3348a21ac6/_pytest/runner.py#L340), i.e. when r.outcome is failed for when in ('setup', 'teardown') then it is a more unexpected error.
This is relevant/helpful with exit_code=2 I guess.
Won't have time for it right now / today though anymore.

[rob-smallshire]

I propose we merge this as is and iterate more later. Clearly we're still in the learning phase here, and I think there's a lot to be gained by merging and using this.

[blueyed]

Ok, fixed the __init__ method from base class 'Exception' is not called (super-init-not-called).

Please consider squash-merging it then.

[rob-smallshire]

Oh, this is absurd! Now the build is failing just because pytest.org happens to be down right now.

[blueyed]

Can you restart it (just for postery)? https://travis-ci.org/sixty-north/cosmic-ray/jobs/308106781

[blueyed]

For reference: pytest 3.3.0 uses exit code 1 now with -x, too: pytest-dev/pytest#2845.

[abingham]

Can you restart it

Unfortunately, I think we can't re-run that since the ref has been removed.