Skip to content
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

No introspective way to detect ModuleImportFailure in unittest #63945

Closed
rbtcollins opened this issue Nov 24, 2013 · 11 comments
Closed

No introspective way to detect ModuleImportFailure in unittest #63945

rbtcollins opened this issue Nov 24, 2013 · 11 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@rbtcollins
Copy link
Member

rbtcollins commented Nov 24, 2013

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
    Copy link
    Member Author

    rbtcollins commented Nov 24, 2013

    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.

    @rbtcollins rbtcollins added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Nov 24, 2013
    @voidspace
    Copy link
    Contributor

    voidspace commented Nov 24, 2013

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

    @ncoghlan ncoghlan changed the title No introspective way to detect ModuleImportFailure No introspective way to detect ModuleImportFailure in unittest Nov 25, 2013
    @rbtcollins
    Copy link
    Member Author

    rbtcollins commented Sep 8, 2014

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

    @bitdancer
    Copy link
    Member

    bitdancer commented Sep 10, 2014

    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
    Copy link
    Member Author

    rbtcollins commented Sep 10, 2014

    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
    Copy link
    Member

    bitdancer commented Sep 10, 2014

    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
    Copy link
    Member Author

    rbtcollins commented Sep 23, 2014

    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
    Copy link
    Member

    bitdancer commented Sep 23, 2014

    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
    Copy link
    Member Author

    rbtcollins commented Sep 25, 2014

    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
    Copy link
    Mannequin

    python-dev mannequin commented Oct 20, 2014

    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

    @python-dev python-dev mannequin closed this as completed Oct 20, 2014
    @peterjc
    Copy link
    Mannequin

    peterjc mannequin commented Sep 14, 2015

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants