#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: yakky, 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 yakky 11 months ago.
Code fix
0002-Test-for-22478.patch (3.7 KB) - added by yakky 11 months ago.
Test
0001-Fixed-22478.-Use-correct-module-variable-in-build_te.2.patch (4.2 KB) - added by yakky 11 months ago.
Code fix + Test

Download all attachments as: .zip

Change History (19)

comment:1 Changed 11 months ago by yakky

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

Tests on the way

Last edited 11 months ago by yakky (previous) (diff)

Changed 11 months ago by yakky

Test

comment:2 follow-up: Changed 11 months ago by yakky

  • Needs tests unset

Test attached

comment:3 Changed 11 months ago by yakky

  • Cc yakky added
  • Keywords tests test suite added

comment:4 Changed 11 months ago by rtnpro

  • Cc rtnpro added

comment:5 Changed 11 months ago by rtnpro

  • Component changed from Uncategorized to Testing framework

comment:6 Changed 11 months ago by rtnpro

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 11 months ago by prestontimmons

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 ; follow-up: Changed 11 months ago by prestontimmons

Replying to yakky:

Test attached

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

comment:9 in reply to: ↑ 8 Changed 11 months ago by yakky

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

comment:10 Changed 11 months ago by yakky

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

comment:11 Changed 11 months 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 11 months ago by yakky

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

comment:13 Changed 11 months ago by rtnpro

  • Triage Stage changed from Accepted to Ready for checkin

comment:14 Changed 11 months ago by prestontimmons

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 11 months ago by yakky

Good, thanks for support!

comment:16 Changed 10 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

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