-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed #35402 -- Fixed crash when DatabaseFeatures.django_test_skips references a class in another test module #18103
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ⛵️!
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",
},
} |
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 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. |
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. |
Checking for "test" seems reasonable since unittest find tests that way, however, the question is why does |
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. |
7a0ff1e
to
a0b3a19
Compare
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. |
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 |
…eferences a class in another test module 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
3cb0d30
to
a2e3eed
Compare
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. |
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? |
Thanks for the investigation @jonmcfee03. I had a look myself. I don't think
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 |
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'>` |
I think it would be reasonable to enhance 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 |
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. |
django/utils/module_loading.py
Outdated
@@ -13,6 +13,11 @@ def cached_import(module_path, class_name): | |||
and getattr(spec, "_initializing", False) is False | |||
): | |||
module = import_module(module_path) | |||
if not hasattr(module, class_name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I had a look at what else might break if we start changing I've already found one case where an informative ImportError is transformed to a less informative TypeError. I'll open a PR with the test case. (#18263) File "/Users/jwalls/django/django/contrib/auth/__init__.py", line 23, in load_backend
return import_string(path)()
^^^^^^^^^^^^^^^^^^^^^
TypeError: 'module' object is not callable
---------------------------------------------------------------------- This suggests to me that we if we go down this path, we should audit the other call sites. Is it worth it? |
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
main
branch.