Opened 2 years ago

Closed 2 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 2 years ago.
Code fix
0002-Test-for-22478.patch (3.7 KB) - added by Iacopo Spalletti 2 years ago.
Test
0001-Fixed-22478.-Use-correct-module-variable-in-build_te.2.patch (4.2 KB) - added by Iacopo Spalletti 2 years ago.
Code fix + Test

Download all attachments as: .zip

Change History (19)

Changed 2 years ago by Iacopo Spalletti

Code fix

comment:1 Changed 2 years ago by Iacopo Spalletti

Has patch: set
Needs documentation: unset
Needs tests: set
Patch needs improvement: unset

Tests on the way

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

Changed 2 years ago by Iacopo Spalletti

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

Test

comment:2 Changed 2 years ago by Iacopo Spalletti

Needs tests: unset

Test attached

comment:3 Changed 2 years ago by Iacopo Spalletti

Cc: Iacopo Spalletti added
Keywords: tests test suite added

comment:4 Changed 2 years ago by rtnpro

Cc: rtnpro added

comment:5 Changed 2 years ago by rtnpro

Component: UncategorizedTesting framework

comment:6 Changed 2 years ago by rtnpro

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 Changed 2 years ago by Preston Timmons

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.

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

Replying to yakky:

Test attached

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

comment:9 in reply to:  8 Changed 2 years ago by Iacopo Spalletti

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

Changed 2 years ago by Iacopo Spalletti

Code fix + Test

comment:10 Changed 2 years ago by Iacopo Spalletti

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

comment:11 Changed 2 years ago by rtnpro

Hey yakky,

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

Regards,
rtnpro

comment:12 Changed 2 years ago by Iacopo Spalletti

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

comment:13 Changed 2 years ago by rtnpro

Triage Stage: AcceptedReady for checkin

comment:14 Changed 2 years ago by Preston Timmons

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 Changed 2 years ago by Iacopo Spalletti

Good, thanks for support!

comment:16 Changed 2 years ago by Tim Graham <timograham@…>

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