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

unittest TestResult wasSuccessful returns True when there are unexpected successes #64364

Closed
gpshead opened this issue Jan 7, 2014 · 8 comments
Assignees
Labels
type-bug An unexpected behavior, bug, or error

Comments

@gpshead
Copy link
Member

gpshead commented Jan 7, 2014

BPO 20165
Nosy @gpshead, @voidspace, @serhiy-storchaka
Files
  • issue20165-gps01.diff
  • 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 = 'https://github.com/gpshead'
    closed_at = <Date 2014-01-20.09:13:00.112>
    created_at = <Date 2014-01-07.19:25:21.692>
    labels = ['type-bug']
    title = 'unittest TestResult wasSuccessful returns True when there are unexpected successes'
    updated_at = <Date 2021-12-16.11:56:09.259>
    user = 'https://github.com/gpshead'

    bugs.python.org fields:

    activity = <Date 2021-12-16.11:56:09.259>
    actor = 'serhiy.storchaka'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2014-01-20.09:13:00.112>
    closer = 'gregory.p.smith'
    components = []
    creation = <Date 2014-01-07.19:25:21.692>
    creator = 'gregory.p.smith'
    dependencies = []
    files = ['33363']
    hgrepos = []
    issue_num = 20165
    keywords = ['patch']
    message_count = 8.0
    messages = ['207582', '207583', '207614', '207629', '207677', '207734', '208530', '408695']
    nosy_count = 5.0
    nosy_names = ['gregory.p.smith', 'michael.foord', 'python-dev', 'serhiy.storchaka', 'slattarini']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue20165'
    versions = ['Python 3.4']

    @gpshead
    Copy link
    Member Author

    gpshead commented Jan 7, 2014

    Python 3.3.3+ (3.3:28337a8fb502+, Jan  7 2014, 01:32:44)
    [GCC 4.6.3] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import unittest
    >>> r = unittest.result.TestResult()
    >>> r.wasSuccessful()
    True
    >>> r.addUnexpectedSuccess("weird!")
    >>> r.wasSuccessful()
    True

    An unexpected success is not a good thing and indicates a problem.

    the wasSuccessful() method should include a check for len(self.unexpectedSuccesses) == 0 as part of its condition.

    @gpshead gpshead self-assigned this Jan 7, 2014
    @gpshead gpshead added the type-bug An unexpected behavior, bug, or error label Jan 7, 2014
    @gpshead
    Copy link
    Member Author

    gpshead commented Jan 7, 2014

    @voidspace
    Copy link
    Contributor

    I'm pretty sure this has been debated before (and the status quo is the result). Trying to find the issue.

    @voidspace
    Copy link
    Contributor

    Hmmm... TestTools has wasSuccessful return False on an unexpected success [1] and I can't find an issue for any previous discussion.

    I don't use unexpected success, so I have no particular horse in this race but it seems more logical that wasSuccessful should return False when there are unexpected successes.

    [1] https://code.launchpad.net/~jml/testtools/unexpected-success-2/+merge/42050

    @gpshead
    Copy link
    Member Author

    gpshead commented Jan 8, 2014

    I'm not comfortable changing this for 2.7 or 3.3 in case some code is unfortunately is depending on this behavior. But as it is it does seem like the kind of thing that can hide problems (tests that are passing that are not expected to).

    Here's a patch for 3.4 (minus the documentation update that'll need to explicitly mention this with a versionchanged::).

    @slattarini
    Copy link
    Mannequin

    slattarini mannequin commented Jan 9, 2014

    Since I too was bitten by this issue, I'd like to support Gregory's
    request, and report my rationale for changing the current behaviour.

    With the current behaviour, we could see (and I *have* seen) scenarios
    like this:

    1. A test exposing a known bug is written, and the test is marked
      as "expected failure".

    2. Some refactoring and code improvements follow.

    3. They improve the overall behaviour/correctness of the program
      or library under test, to the point of fixing the bug "behind
      the scenes".

    4. The developer doesn't notice the fix though, since the testing
      framework doesn't warn him "strongly enough" of the fact that the
      test marked as expected failure has begun to pass. (To reiterate,
      the current behaviour of just printing a warning saying "some test
      passed unexpectedly" on the standard output is not good enough of
      a warning; it's easy to miss, and worse, it's *certain* that it
      will be missed if the tests are run by some CI systems or a similar
      wrapper system -- those would only report failures due to non-zero
      exit statuses.)

    5. Without noticing that the issue has been fixed, the developer does
      some further changes, which again re-introduce the bug, or a
      similar one that the test still marked as "expected failure" could
      easily catch.

    6. That test starts to fail again; but since it has remained marked
      as "expected failure" all along, the fact is not reported to the
      developer in any way. So the bug has managed to sneak back in,
      unnoticed.

    In addition to this rationale, another (weaker) reason to change the
    existing behaviour would be the "principle of least surprise". Among
    other widely used testing framework (for python, or language-agnostic)
    many of those which support the concept of "expected failure" will
    throw hard errors by default when a test marked as expected failure
    starts to pass; among these are:

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 20, 2014

    New changeset 1e75ab9fd760 by Gregory P. Smith in branch 'default':
    Fixes Issue bpo-20165: The unittest module no longer considers tests marked with
    http://hg.python.org/cpython/rev/1e75ab9fd760

    @gpshead gpshead closed this as completed Jan 20, 2014
    @serhiy-storchaka
    Copy link
    Member

    See also bpo-22815.

    @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
    type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants