No introspective way to detect ModuleImportFailure in unittest

[rbtcollins]

BPO	19746
Nosy	@vstinner, @rbtcollins, @bitdancer, @voidspace, @peterjc
Files	issue19746.patch issue19746-rebased.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2014-10-20.00:24:38.148>
created_at = <Date 2013-11-24.03:16:03.568>
labels = ['type-feature', 'library']
title = 'No introspective way to detect ModuleImportFailure in unittest'
updated_at = <Date 2015-09-14.11:38:46.218>
user = 'https://github.com/rbtcollins'

bugs.python.org fields:

activity = <Date 2015-09-14.11:38:46.218>
actor = 'maubp'
assignee = 'none'
closed = True
closed_date = <Date 2014-10-20.00:24:38.148>
closer = 'python-dev'
components = ['Library (Lib)']
creation = <Date 2013-11-24.03:16:03.568>
creator = 'rbcollins'
dependencies = []
files = ['36577', '36587']
hgrepos = []
issue_num = 19746
keywords = ['patch']
message_count = 11.0
messages = ['204172', '204265', '226612', '226671', '226674', '226693', '227318', '227363', '227500', '229704', '250656']
nosy_count = 6.0
nosy_names = ['vstinner', 'rbcollins', 'r.david.murray', 'michael.foord', 'maubp', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue19746'
versions = ['Python 3.5']

[rbtcollins]

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.

[voidspace]

Seems like a perfectly reasonable request. I've no particular preference on an api for this though.

[rbtcollins]

Here is an implementation. I'm probably missing some finesse in the docs.

[bitdancer]

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 bpo-7559 patch, but since it based on this one I don't think I can just rebase it to review it.)

[rbtcollins]

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?

[bitdancer]

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.

[rbtcollins]

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.

[bitdancer]

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.

[rbtcollins]

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.

[python-dev]

New changeset e906e23931fa by Robert Collins in branch 'default':
Close bpo-19746: expose unittest discovery errors on TestLoader.errors
https://hg.python.org/cpython/rev/e906e23931fa

[peterjc]

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:
biopython/biopython@929fbfb

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).