Opened 11 years ago

Closed 11 years ago

#22478 closed Bug (fixed)

Wrong module name used in build_test

Reported by: anonymous Owned by: nobody
Component: Testing framework Version: 1.7-beta-1
Severity: Normal Keywords: tests, test suite
Cc: Iacopo Spalletti, rtnpro Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

TestClass loading in django.test.simple.build_test cycle through discovered modules in application, but iterated modules are not used in the for cycle.

See https://github.com/django/django/blob/stable/1.7.x/django/test/simple.py#L183

Attachments (3)

0001-Fixed-22478.-Use-correct-module-variable-in-build_te.patch (751 bytes ) - added by Iacopo Spalletti 11 years ago.
Code fix
0002-Test-for-22478.patch (3.7 KB ) - added by Iacopo Spalletti 11 years ago.
Test
0001-Fixed-22478.-Use-correct-module-variable-in-build_te.2.patch (4.2 KB ) - added by Iacopo Spalletti 11 years ago.
Code fix + Test

Download all attachments as: .zip

Change History (19)

comment:1 by Iacopo Spalletti, 11 years ago

Has patch: set
Needs tests: set

Tests on the way

Last edited 11 years ago by Iacopo Spalletti (previous) (diff)

by Iacopo Spalletti, 11 years ago

Attachment: 0002-Test-for-22478.patch added

Test

comment:2 by Iacopo Spalletti, 11 years ago

Needs tests: unset

Test attached

comment:3 by Iacopo Spalletti, 11 years ago

Cc: Iacopo Spalletti added
Keywords: tests test suite added

comment:4 by rtnpro, 11 years ago

Cc: rtnpro added

comment:5 by rtnpro, 11 years ago

Component: UncategorizedTesting framework

comment:6 by rtnpro, 11 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

I am able to reproduce the reported bug. I have also reviewed the patches and tested it on my local codebase of Django running tag 1.7b1. The patches seem to be working, however, they are not PEP8 compliant.

$ pep8 tests/test_suite_override/sample_app/
tests/test_suite_override/sample_app/__init__.py:1:24: W292 no newline at end of file
tests/test_suite_override/sample_app/models.py:4:1: E302 expected 2 blank lines, found 1
tests/test_suite_override/sample_app/tests/__init__.py:2:27: W292 no newline at end of file

So, I'd suggest yakky to make the change PEP8 compliant and submit an incremental patch.

I will mark this ticket as accepted. If someone has any point to add, please feel free to share your opinion.

comment:7 by Preston Timmons, 11 years ago

The issue wasn't clear to me based on the report. It boiled down to this:

If you define a test in tests/__init__.py, and use the old test runner, a test label like myapp.TestCase fails to resolve to the test case. Instead, a ValueError is raised:

ValueError: Test label 'testapp.MyTest' does not refer to a test

From my testing, this works in the 1.6.x branch but fails in 1.7.x. I didn't track down the commit that caused the regression.

in reply to:  2 ; comment:8 by Preston Timmons, 11 years ago

Replying to yakky:

Test attached

yakky, it would help if you submitted this as a single patch.

in reply to:  8 comment:9 by Iacopo Spalletti, 11 years ago

Replying to prestontimmons:

Replying to yakky:

Test attached

yakky, it would help if you submitted this as a single patch.

Sure.
Will make the code compliancy modifications and resubmit as single patch

by Iacopo Spalletti, 11 years ago

Code fix + Test

comment:10 by Iacopo Spalletti, 11 years ago

Sorry, quite new here: is it possible to remove obsolete attachments?

comment:11 by rtnpro, 11 years ago

Hey yakky,

AFAIK, you can replace existing attachment with the same name, but not with a different name.

Regards,
rtnpro

comment:12 by Iacopo Spalletti, 11 years ago

Thanks.
0001-Fixed-22478.-Use-correct-module-variable-in-build_te.2.patch is the complete patch (code -linted- and test)

comment:13 by rtnpro, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:14 by Preston Timmons, 11 years ago

Thanks, yakky. That illustrates the problem well.

I added a pull request here: https://github.com/django/django/pull/2596

I simplified your test case, since test_suite_override isn't really the right directory for this test, and there is a sample app already with an __init__.py.

Thanks for preparing this.

comment:15 by Iacopo Spalletti, 11 years ago

Good, thanks for support!

comment:16 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 935159d9517ca951cbd5665c521d3c91102948d8:

[1.7.x] Fixed #22478 -- Regression in test label discovery.

As part of the app-loading updates the old test runner was changed to not
require a models module. This introduced a regression in behavior so
applabel.TestCase failed for tests defined in a directory.

The fix is thanks to yakky and rtnpro.

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