classification
Title: No introspective way to detect ModuleImportFailure in unittest
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, maubp, michael.foord, python-dev, r.david.murray, rbcollins
Priority: normal Keywords: patch

Created on 2013-11-24 03:16 by rbcollins, last changed 2015-09-14 11:38 by maubp. This issue is now closed.

Files
File name Uploaded Description Edit
issue19746.patch rbcollins, 2014-09-08 21:27
issue19746-rebased.patch r.david.murray, 2014-09-10 01:11 review
Messages (11)
msg204172 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2013-11-24 03:16
https://bugs.launchpad.net/testtools/+bug/1245672 was filed on testtools recently. It would be easier to fix that if there was some way that something loading a test suite could check to see if there were import errors. The current code nicely works in the case where folk run the tests - so we should keep that behaviour, but also accumulate a list somewhere.

One possibility would be on the returned top level suite; another place would be on the loader itself. Or a new return type where we return a tuple of (suite, failures), but thats a more intrusive API change.

Any preference about how to solve this? I will work up a patch given some minor direction.
msg204265 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2013-11-24 20:33
Seems like a perfectly reasonable request. I've no particular preference on an api for this though.
msg226612 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-09-08 21:27
Here is an implementation. I'm probably missing some finesse in the docs.
msg226671 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-09-10 01:36
Your patch isn't diffed against a revision from the cpython repo, and apparently didn't apply cleanly to tip, so no review link was generated.  I  uploaded a rebased patch to review, but don't actually have any line by line comments.

You are right about the docs.  Reading that, I thought it was saying that errors would have a list of the errors that show up in the summary as Errors=N, and not the ones that show up as Failures=N, which of course is completely off base for two reasons (but, then, I can never remember the difference between Failure and Error and always ignore what type the test failures are).

Anyway, you probably want to talk about actual error types.  I understand ImportError, but I have no idea what would trigger the 'Failed to call load_tests' error.  Nor is it clear what would be a fatal error (easier just to say which errors are trapped, I think). It should also be mentioned that the contents of the list are an error message followed by a full traceback.  And, frankly, I'm not sure why this is useful, and in particular why this implementation satisfies your use case.

(I'm more interested in the issue 7559 patch, but since it based on this one I don't think I can just rebase it to review it.)
msg226674 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-09-10 02:26
Thanks; I'm still learning how to get the system here to jump appropriately :). I thought I'd told hg to reset me to trunk...

"You are right about the docs.  Reading that, I thought it was saying that errors would have a list of the errors that show up in the summary as Errors=N, and not the ones that show up as Failures=N, which of course is completely off base for two reasons (but, then, I can never remember the difference between Failure and Error and always ignore what type the test failures are)."

Ah, so this is specifically about *loader* errors, nothing to do with the ERROR and FAILURE concepts for the TestResult class; that definitely needs to be made more clear.

"Anyway, you probably want to talk about actual error types.  I understand ImportError, but I have no idea what would trigger the 'Failed to call load_tests' error.  Nor is it clear what would be a fatal error (easier just to say which errors are trapped, I think)."

'Failed to call load_tests' is an existing error that can be triggered if a load_tests method errors.

e.g. put this in a test_foo.py:

def load_tests(loader, tests, pattern):
    raise Exception('fred')

to see it with/without the patch. I'll take a stab at improving the docs in a bit.

"It should also be mentioned that the contents of the list are an error message followed by a full traceback.  And, frankly, I'm not sure why this is useful, and in particular why this implementation satisfies your use case."

Ah! So I have an external runner which can enumerate the tests without running them. This is useful when the test suite is being distributed across many machines (simple hash based distribution can have very uneven running times, so enumerating the tests that need to run then scheduling based on runtime (or other metadata) gets better results). If the test suite can't be imported I need to show the failure of the import to users so they can fix it, but since the test suite may take hours (or even not be runnable locally) I need to do this without running the tests. Thus a simple list of the tracebacks encountered loading the test suite is sufficient. Where would be a good place to make this clearer?
msg226693 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-09-10 12:42
Yeah, I figured out it was loader only errors after I read the code :)

"load_tests not called" is very different from "load_tests produced an exception", so the text of the error message should be changed accordingly.

I understood your use case more-or-less, my question was, why is the error *string* the thing returned?  Given that the synthetic test encapsulates and re-raises the error, might it be more useful to store the exception object in the errors attribute rather than the message strings?  That would seem to provide more introspectability.
msg227318 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-09-23 00:29
I can certainly write the reporter glue to work with either a string or a full reference. Note that the existing late-reporting glue captures the import error into a string, and then raises an exception containing that string - so what I've done is consistent with that.

As for why the original code is like that - well I've had plenty of bad experiences with memory loops due to objects held in stack frames of exceptions, I don't like to keep long lived references to them, and I wager Michael has had the same experience.
msg227363 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-09-23 14:55
Oh, ok, if the existing glue does it that way, then it seems fine.  I thought when I read the code that it was holding a reference to the traceback until it raised the error in the synthetic test.  Or do you mean that when exceptions are raised by tests it is the string that is captured not the exception?  That would make sense.  So, yeah, I guess capturing the string makes sense, to be consistent with the rest of the reporting.
msg227500 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-09-25 01:26
Right: the existing code stringifies the original exception and creates an exception object and a closure
def test_thing(self):
    raise exception_obj

but that has the stringified original exception.
msg229704 - (view) Author: Roundup Robot (python-dev) Date: 2014-10-20 00:24
New changeset e906e23931fa by Robert Collins in branch 'default':
Close #19746: expose unittest discovery errors on TestLoader.errors
https://hg.python.org/cpython/rev/e906e23931fa
msg250656 - (view) Author: Peter (maubp) Date: 2015-09-14 11:38
This comment is just to note that this change broke our (exotic?) usage of unittest.TestLoader().loadTestsFromName(name) inside a try/except since under Python 3.5 some expected exceptions are no longer raised.

My nasty workaround hack:
https://github.com/biopython/biopython/commit/929fbfbcf2d1ba65ec460a413128dd5e6f68f5bf

I think it is unfortunate that the .errors attribute is just a list of messages as strings, rather than the original exception object(s) which would be easier to handle (e.g. using the subclass hierarchy).
History
Date User Action Args
2015-09-14 11:38:46maubpsetnosy: + maubp
messages: + msg250656
2014-10-20 00:24:38python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg229704

resolution: fixed
stage: resolved
2014-09-25 01:26:27rbcollinssetmessages: + msg227500
2014-09-23 14:55:41r.david.murraysetmessages: + msg227363
2014-09-23 00:29:05rbcollinssetmessages: + msg227318
2014-09-10 12:42:16r.david.murraysetmessages: + msg226693
2014-09-10 02:26:24rbcollinssetmessages: + msg226674
2014-09-10 01:36:22r.david.murraysetnosy: + r.david.murray
messages: + msg226671
2014-09-10 01:11:06r.david.murraysetfiles: + issue19746-rebased.patch
2014-09-08 21:27:16rbcollinssetfiles: + issue19746.patch
keywords: + patch
messages: + msg226612
2014-04-08 08:33:50hayposetnosy: + haypo
2013-11-25 05:02:10ncoghlansettitle: No introspective way to detect ModuleImportFailure -> No introspective way to detect ModuleImportFailure in unittest
2013-11-24 20:33:58michael.foordsetmessages: + msg204265
2013-11-24 03:16:03rbcollinscreate