Opened 6 months ago
Last modified 8 weeks ago
#35402 assigned Bug
DatabaseFeatures.django_test_skips crashes when it references a class in another test module
Reported by: | Tim Graham | Owned by: | jon-mcfee |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Since the introduction of DatabaseFeatures.django_test_skips
(#32178), I've used it to skip test classes, however, this can crash if there are multiple test modules in a package.
To reproduce, first make this modification to skip a class
-
django/db/backends/sqlite3/features.py
diff --git a/django/db/backends/sqlite3/features.py b/django/db/backends/sqlite3/features.py index d95c6fb2d1..51229fe970 100644
a b class DatabaseFeatures(BaseDatabaseFeatures): 68 68 def django_test_skips(self): 69 69 skips = { 70 70 "SQLite stores values rounded to 15 significant digits.": { 71 "model_fields.test_decimalfield.DecimalFieldTests." 72 "test_fetch_from_db_without_float_rounding", 71 "model_fields.test_decimalfield.DecimalFieldTests", 73 72 }, 74 73 "SQLite naively remakes the table on field alteration.": {
Then try to execute a test module in model_fields
besides the one with the skip:
$ ./tests/runtests.py model_fields.test_autofield Testing against Django installed in '/home/tim/code/django/django' with up to 3 processes Found 62 test(s). Creating test database for alias 'default'... Traceback (most recent call last): File "/home/tim/code/django/django/utils/module_loading.py", line 30, in import_string return cached_import(module_path, class_name) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/tim/code/django/django/utils/module_loading.py", line 16, in cached_import return getattr(module, class_name) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ AttributeError: module 'model_fields' has no attribute 'test_decimalfield' The above exception was the direct cause of the following exception: Traceback (most recent call last): File "/home/tim/code/django/./tests/runtests.py", line 784, in <module> failures = django_tests( ^^^^^^^^^^^^^ File "/home/tim/code/django/./tests/runtests.py", line 422, in django_tests failures = test_runner.run_tests(test_labels) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/tim/code/django/django/test/runner.py", line 1066, in run_tests old_config = self.setup_databases( ^^^^^^^^^^^^^^^^^^^^^ File "/home/tim/code/django/django/test/runner.py", line 964, in setup_databases return _setup_databases( ^^^^^^^^^^^^^^^^^ File "/home/tim/code/django/django/test/utils.py", line 206, in setup_databases connection.creation.create_test_db( File "/home/tim/code/django/django/db/backends/base/creation.py", line 102, in create_test_db self.mark_expected_failures_and_skips() File "/home/tim/code/django/django/db/backends/base/creation.py", line 356, in mark_expected_failures_and_skips test_case = import_string(test_case_name) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/tim/code/django/django/utils/module_loading.py", line 32, in import_string raise ImportError( ImportError: Module "model_fields" does not define a "test_decimalfield" attribute/class
A naive fix:
-
django/db/backends/base/creation.py
diff --git a/django/db/backends/base/creation.py b/django/db/backends/base/creation.py index 6856fdb596..f6cc270b16 100644
a b class BaseDatabaseCreation: 350 350 test_app = test_name.split(".")[0] 351 351 # Importing a test app that isn't installed raises RuntimeError. 352 352 if test_app in settings.INSTALLED_APPS: 353 # If this is a a test class, it may need to be imported. 354 if test_name.count(".") == 2: 355 import_string(test_name) 353 356 test_case = import_string(test_case_name) 354 357 test_method = getattr(test_case, test_method_name) 355 358 setattr(test_case, test_method_name, skip(reason)(test_method))
Change History (10)
comment:1 by , 6 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 6 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 6 months ago
In the backends.base.test_creation.TestMarkTests
, it seems like there is a test for skipping a class.
"skip test class": { "backends.base.test_creation.SkipTestClass", }
Interestingly, this passes, yet the ImportError
is still thrown for seemingly the same behavior in django/db/backends/base/features.py
. I can't really understand why import_string
accepts "backends.base.test_creation"
, but fails on "model_fields.test_decimalfield"
. Can anyone offer any advice?
comment:4 by , 6 months ago
I believe the problem has something to do with module loading. When running only model_fields.test_autofield
, model_fields.test_decimalfield
is not loaded, thus the import error. If I understood the problem completely, I would have offered the fix.
comment:7 by , 6 months ago
Okay, I made a PR. I'm not sure if it's naive to assume that tests must start with "test", but I passed all local tests and automated testing, so please let me know if that should work.
comment:9 by , 4 months ago
Needs documentation: | set |
---|---|
Owner: | changed from | to
Un-commandeering the issue ;)
Needs an update to the docs for import_string()
.
comment:10 by , 3 months ago
Needs documentation: | unset |
---|
PR to cover uncovered lines related to the proposed fix. May help inform decision making about the eventual solution.
comment:12 by , 8 weeks ago
Patch needs improvement: | set |
---|
Thank you for the report!