Fixed #36479: Failing test for black formatter missing install simulation on a macOS #19591
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 ⛵️!
tests/user_commands/tests.py
Outdated
| (FileNotFoundError, "nonexistent"), | ||
| ( | ||
| NotADirectoryError if sys.platform == "darwin" else FileNotFoundError, | ||
| "nonexistent", | ||
| ), |
There was a problem hiding this comment.
This change causes a failure for me, on a Mac (darwin). It should probably just be OSError.
There was a problem hiding this comment.
Hey @jacobtylerwalls, thanks for the input. Can you let me know what error you get?
If we really can't rely on a more specific error class, then maybe we can just use the OSError just to be more generic.
There was a problem hiding this comment.
Sure thing, I get FileNotFoundError:
======================================================================
FAIL: test_run_formatters_handles_oserror_for_black_path (user_commands.tests.UtilsTests.test_run_formatters_handles_oserror_for_black_path) [NotADirectoryError]
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/jwalls/django/tests/user_commands/tests.py", line 587, in test_run_formatters_handles_oserror_for_black_path
self.assertIn(exception.__qualname__, parsed_error)
~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'NotADirectoryError' not found in 'Formatters failed to launch:Traceback (most recent call last):\n File "/Users/jwalls/django/django/core/management/utils.py", line 175, in run_formatters\n subprocess.run(\n ~~~~~~~~~~~~~~^\n [black_path, "--fast", "--", *written_files],\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n capture_output=True,\n ^^^^^^^^^^^^^^^^^^^^\n )\n ^\n File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/subprocess.py", line 554, in run\n with Popen(*popenargs, **kwargs) as process:\n ~~~~~^^^^^^^^^^^^^^^^^^^^^^\n File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/subprocess.py", line 1036, in __init__\n self._execute_child(args, executable, preexec_fn, close_fds,\n ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n pass_fds, cwd, env,\n ^^^^^^^^^^^^^^^^^^^\n ...<5 lines>...\n gid, gids, uid, umask,\n ^^^^^^^^^^^^^^^^^^^^^^\n start_new_session, process_group)\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/subprocess.py", line 1966, in _execute_child\n raise child_exception_type(errno_num, err_msg, err_filename)\nFileNotFoundError: [Errno 2] No such file or directory: \'nonexistent\'\n'
----------------------------------------------------------------------
Ran 48 tests in 1.815s
FAILED (failures=1)There was a problem hiding this comment.
Using both macOS and having different exceptions for this test is a bit bothersome 😄, we can't use OSError because the test looks for the class name found in the traceback stderr produced by the run_formatters, in our case, it wont be able find the OSError class name.
Unless we really wanna control the stderr.write message within the run_formatters with common information, it is not possible to have a generic criteria 😬 .
The other alternative on top of my head is instead of having a limited/fixed expected class paired with the location. Why don't we use the list of definite classes that derived from OSError and iterate to check if one of the errors is found in the generated stderr?:
os_errors = (OSError, NotADirectoryError, FileNotFoundError, PermissionError)
locations = (
"nonexistent",
str(Path(__file__).parent / "test_files" / "black"),
)
for location in locations:
with (
self.subTest(location),
AssertFormatterFailureCaughtContext(
self, shutil_which_result=location
) as ctx,
):
...
self.assertTrue(
any(os_error.__qualname__ in parsed_error for os_error in os_errors)
)
...What do you think about it?
There was a problem hiding this comment.
Thanks for looking into it. I think we should revisit the decision to assert against the qualname in the test. I don't think it's Django's job to test Python's exception message wording. (I had a passing comment along these lines when we developed this test.) We're just trying to cover that traceback.print_exc() is called. We could assert that Traceback (most recent call last): is present, since that's documented.
I think just asserting OSError is raised is good enough: that's how we described the issue in the description for #18597.
If we decide to keep asserting against the qualname, we should iterate the qualnames of the classes found in the .mro() of the exception class instead of hard-coding os_errors = ...
There was a problem hiding this comment.
This is nice information, thank you for mentioning these links. The only blocker that prevents me from asserting OSError or getting the raised exception class information is simply because I ran out of ideas on how to collect the information since the OSError is suppressed here. I'm still looking for a simple solution.
EDIT:
By the way, I also notice that the test is passing with just using the FileNotFoundError for the nonexistent string as long as it is formatted as a path.
str(Path(__file__).parent / "test_files" / "nonexistent")"/nonexistent"
So the following tests passed:
(
FileNotFoundError,
str(Path(__file__).parent / "test_files" / "nonexistent"),
# or "/nonexistent",
)If this is not ideal, I guess we can just leave the test :)
…st case for FileNotFound.
roelzkie15
changed the title
roelzkie15
force-pushed
the
fix/ticket-36479-failing-missing-black-install-simulation-test
branch
from
27b1150 to
d7f555c
Compare
Trac ticket number
ticket-36479
Branch description
This branch added a simple fix for running
./runtests.py user_commands.tests.UtilsTests.test_run_formatters_handles_oserror_for_black_pathwhich fails on adarwinplatform but passes on alinuxplatform due to different subclass results yielded byOSErrorfrom different platforms. Please see the ticket for further details of the problem.Checklist
mainbranch.