Fixed #35402 -- Fixed crash when DatabaseFeatures.django_test_skips references a class in another test module

[jonmcfee03]

Trac ticket number

ticket-35402

Branch description

Previously, if a submodule skipped test classes in an adjacent submodule (same
parent module), only the running submodule was imported and an error was thrown
because getattr would not find the submodule of the to-be-skipped class. Updated
cached_import in django/utils/module_loading.py to include a check for the skipped
submodule, and import it if it is not there.

Thanks to Tim Graham for reporting this bug

Checklist

• This PR targets the main branch.
• The commit message is written in past tense, mentions the ticket number, and ends with a period.
• I have checked the "Has patch" ticket flag in the Trac system.
• I have added or updated relevant tests.
• I have added or updated relevant docs, including release notes if applicable.
• I have attached screenshots in both light and dark modes for any UI changes.

[github-actions]

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

[jonmcfee03]

Should I add another test that tests skipping another class in a different module? For example:

creation.connection.features.django_test_skips = {
      "skip test class": {
          "backends.base.test_creation.SkipTestClass",
      },
      "skip test class in another module": {
          "backends.base.test_client.SimpleDatabaseClientTests"
      },
      "skip test function": {
          "backends.base.test_creation.test_skip_test_function",
      },
  }

[timgraham]

The fix works but I still don't understand the "why" of the issue. I wonder if you would like to read more about Python's import process so you might be able to explain the root of the issue to ensure we shouldn't be fixing this is some other way, like an enhancement to django.utils.module_loading.import_string perhaps.

It would be worth adding a regression test if it's possible -- I'm not sure that's the case since I can only reproduce the problem in my situation when running a subset of the tests.

[jonmcfee03]

The test case that I provided in the follow-up does break the old code and works with the new code, so it's a good regression test to add. However, I agree that just adding the check for "test" at the beginning of the string might not be the most robust solution. I'll do some reading to try to identify another solution.

[timgraham]

Checking for "test" seems reasonable since unittest find tests that way, however, the question is why does import_string('model_fields.test_decimalfield') fail when the module exists but hasn't been loaded. I stuck apdb.set_trace() right before the exception occurs and executed import model_fields.test_decimalfield. After doing that, import_string('model_fields.test_decimalfield') succeeds.

[jonmcfee03]

I think I understand why the error is occurring. In your initial example, the module is model_fields.test_autofield is being imported. Now, the model_fields package is imported, but only the test_autofield submodule is included. With the current code structure, if you try to skip JUST a method (i.e. model_fields.test_decimalfields.DecimalFieldTest.test_example), it will check if the model_fields.test_decimalfields module is imported, which it is NOT. However, when an entire class is skipped, it checks if JUST the model_fields module is imported, which it is. HOWEVER, only the test_autofield submodule is imported, not the test_decimalfield, so then it doesn't actually import test_decimalfield and therefore the error is thrown. So, it must check if the actual submodule is imported, too. The new solution is to modify cached_import and check if the submodule is imported using hasattr(). I will submit an updated PR momentarily.

[jonmcfee03]

Also, I'm not familiar with how to update PRs. Please let me know if I have done anything incorrectly. I reverted the changes I made to mark_expected_failures_and_skips, and I also a skip test for skipping a test class in an adjacent submodule.

[timgraham]

Looking good. Try to rebase your branch on main and then squashing your commits. You've inadvertently mixed your changes in with one of Simon's commits. I don't think you should reference backends.base.test_features.TestDatabaseFeatures since that could cause it to actually be skipped. Since the fix is in import_string(), I would instead add a test for it in tests/utils_tests/test_module_loading.py.

[jonmcfee03]

Okay, I believe I've successfully squashed all previous commits into 1, fixed the accidental merge with Simon's commit, and added a regression test to tests/utils_tests/test_module_loading.py that utilizes the test_modules. I have ensured that the test fails before the change, and passes after the change, so I think everything is working properly.

[jonmcfee03]

I think a lot of the failures that are occurring actually have to do with a flaw in the regression test. Can you offer any suggestions on how to create one that works?

[jacobtylerwalls]

Thanks for the investigation @jonmcfee03. I had a look myself.

I don't think import_string() is the issue. Its docstring says:

"Import a dotted module path and return the attribute/class designated by the last name in the path."

For modules, this doesn't make any sense, as the last name in the path needs to be part of the import--there is no attribute or class. I don't think we should alter its behavior to accept modules.

I think the issue is just with this logic sending modules to import_string and hoping for the best.

Alternative PR

[jacobtylerwalls]

however, the question is why does import_string('model_fields.test_decimalfield') fail when the module exists but hasn't been loaded.

import_string() is a bit of a misnomer. It chops off the rightmost term, imports the preceding part, and tries to getattr() the chopped off rightmost term.

So, if the rightmost term is a submodule, this may or may not work depending on whether the submodule was explicitly imported before:

>>> import xml
>>> getattr(xml, "etree")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'xml' has no attribute 'etree'
>>> import xml.etree
>>> getattr(xml, "etree")
<module 'xml.etree' from '/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/xml/etree/__init__.py'>`

[timgraham]

I think it would be reasonable to enhance import_string() so that it works with modules, just like werkzeug.import_string.

If we decide not to, then would you suggest putting the behavior of working with modules only when they are already imported on a deprecation path and eventually prohibit it to avoid this gotcha?

What are we gaining with your more complicated fix rather than letting import_string() do the work?

[jacobtylerwalls]

What are we gaining with your more complicated fix rather than letting import_string() do the work?

Good point. Nothing is gained; just wanted to have a look at an option besides changing the documented behavior. Will close the alternative PR.

@jonmcfee03 You might draw some inspiration from the test in the alternate PR that engineers a situation where a submodule has not already been imported. Thanks for letting me ponder the issue. I'll set you back to the owner of the ticket on Trac.

[jacobtylerwalls]

class_name allowed to also be a submodule name is confusing -- since cached_import() doesn't appear to be documented, I think we can take the opportunity to rename to something like submodule_or_class. Not sure of the best name.

An inline comment here would also be helpful IMO. Otherwise it's not clear that getattr'ing a submodule off the parent module only sometimes works (i.e. when submodule already explicitly imported).

[jacobtylerwalls]

I guess we could also enhance it to take top-level modules and just skip the getattr call. Then it becomes module_or_class. Or maybe "attribute" is enough?

[jonmcfee03]

I agree, with the intended functionality, the names are quite confusing. As this is my first PR, I am not too familiar with Django naming conventions. I think “attribute” would suffice, as that encapsulates all importable items.