Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#12574 closed (fixed)

all tests pass when there broken tests becase they are skipped

Reported by: CarlFK Owned by: nobody
Component: Testing framework Version: dev
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If a test is broken (like typo in models.py)
and you run the full set of tests:

django-trunk/tests$ ./runtests.py --settings=sets_sqlite3

the broken test gets skipped
and so you get:

----------------------------------------------------------------------
Ran 1390 tests in 243.340s
OK
----------------------------------------------------------------------

Which makes you think "oh boy! all my tests passed!" and go dancing in the street.

But no! the very test you were most concerned about never got run.
If you dig around in the output, you will see the error.
If you see OK, you really have no reason to do that.

Here is why:
(trimmed down version)

        try:
            mod = load_app(model_label)
            if mod:
                settings.INSTALLED_APPS.append(model_label)
        except Exception, e:
            sys.stderr.write("Error while importing %s:" % model_name + ''.join(traceback.format_exception(*sys.exc_info())[1:]))

Looking at the change history, it seems this is the result of some refactoring that resulted in this hiding place for bad tests. The "continue" command is unnecessary, given it is at the end of the codeblock.

Seems the fix is to remove the try/except:err.write and let the error get thrown.

btw, the comment for

if not test_labels or model_name in set([label.split('.')[0] for label in test_labels]):

is screwy. so is the code. Neither read well. if you fully understand it, it would be great if it could be cleaned up so I don't get a headache.

Attachments (2)

runtests.diff (1.7 KB ) - added by CarlFK 14 years ago.
12574.diff (1.7 KB ) - added by Ramiro Morales 14 years ago.
Same patch plus moving calculation of the set of test outside of a loop

Download all attachments as: .zip

Change History (6)

by CarlFK, 14 years ago

Attachment: runtests.diff added

comment:1 by CarlFK, 14 years ago

to see the bug in action:

echo foo >> modeltests/empty/models.py
./runtests.py --settings sets_sqlite -v 1

If you do -v 0 (the default), then you will see the stack dump ending in

NameError: name 'foo' is not defined

but also see: OK (which means 'all tests passed') (which is the bug).

by Ramiro Morales, 14 years ago

Attachment: 12574.diff added

Same patch plus moving calculation of the set of test outside of a loop

comment:2 by Eric Holscher, 14 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Russell Keith-Magee, 14 years ago

Resolution: fixed
Status: newclosed

(In [13616]) Fixed #12574 -- Removed an unnecessary exception catch from the system runtest script, which could hide failing tests. Thanks to CarlFK for the report, and Ramiro Morales for the polish.

comment:4 by Russell Keith-Magee, 14 years ago

(In [13617]) [1.2.X] Fixed #12574 -- Removed an unnecessary exception catch from the system runtest script, which could hide failing tests. Thanks to CarlFK for the report, and Ramiro Morales for the polish.

Backport of r13616 from trunk.

Note: See TracTickets for help on using tickets.
Back to Top