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?