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):  
    6868    def django_test_skips(self):
    6969        skips = {
    7070            "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",
    7372            },
    7473            "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:  
    350350                test_app = test_name.split(".")[0]
    351351                # Importing a test app that isn't installed raises RuntimeError.
    352352                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)
    353356                    test_case = import_string(test_case_name)
    354357                    test_method = getattr(test_case, test_method_name)
    355358                    setattr(test_case, test_method_name, skip(reason)(test_method))

Change History (10)

comment:1 by Sarah Boyce, 6 months ago

Triage Stage: UnreviewedAccepted

Thank you for the report!

comment:2 by jon-mcfee, 6 months ago

Owner: changed from nobody to jon-mcfee
Status: newassigned

comment:3 by jon-mcfee, 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?

Last edited 6 months ago by Tim Graham (previous) (diff)

comment:4 by Tim Graham, 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 jon-mcfee, 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:8 by Jacob Walls, 4 months ago

Has patch: set
Owner: set to Jacob Walls

comment:9 by Jacob Walls, 4 months ago

Needs documentation: set
Owner: changed from Jacob Walls to jon-mcfee

Un-commandeering the issue ;)

Needs an update to the docs for import_string().

comment:10 by Jacob Walls, 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:11 by Sarah Boyce <42296566+sarahboyce@…>, 8 weeks ago

In b99c608e:

Refs #35402 -- Added tests for invalid usage of submodules in some settings.

comment:12 by Sarah Boyce, 8 weeks ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top