From 58cc91275a68bb42780cf0e663dad9ecf49039de Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Fri, 29 Nov 2024 07:04:48 -0500 Subject: [PATCH] Fixed #35308 -- Handled OSError when launching code formatters. Co-authored-by: Natalia <124304+nessita@users.noreply.github.com> --- .../management/commands/makemigrations.py | 4 +-- .../management/commands/optimizemigration.py | 2 +- .../management/commands/squashmigrations.py | 2 +- django/core/management/templates.py | 2 +- django/core/management/utils.py | 16 ++++++++---- tests/admin_scripts/tests.py | 12 +++++++++ tests/migrations/test_base.py | 12 +++++++++ tests/migrations/test_commands.py | 21 ++++++++++++++++ tests/user_commands/test_files/black | 1 + tests/user_commands/tests.py | 25 +++++++++++++++++++ tests/user_commands/utils.py | 23 +++++++++++++++++ 11 files changed, 110 insertions(+), 10 deletions(-) create mode 100644 tests/user_commands/test_files/black create mode 100644 tests/user_commands/utils.py diff --git a/django/core/management/commands/makemigrations.py b/django/core/management/commands/makemigrations.py index d5d3466201..690d0e5053 100644 --- a/django/core/management/commands/makemigrations.py +++ b/django/core/management/commands/makemigrations.py @@ -392,7 +392,7 @@ class Command(BaseCommand): ) ) self.log(writer.as_string()) - run_formatters(self.written_files) + run_formatters(self.written_files, stderr=self.stderr) @staticmethod def get_relative_path(path): @@ -499,7 +499,7 @@ class Command(BaseCommand): # Write the merge migrations file to the disk with open(writer.path, "w", encoding="utf-8") as fh: fh.write(writer.as_string()) - run_formatters([writer.path]) + run_formatters([writer.path], stderr=self.stderr) if self.verbosity > 0: self.log("\nCreated new merge migration %s" % writer.path) if self.scriptable: diff --git a/django/core/management/commands/optimizemigration.py b/django/core/management/commands/optimizemigration.py index 2064dfbf3c..90db2d2773 100644 --- a/django/core/management/commands/optimizemigration.py +++ b/django/core/management/commands/optimizemigration.py @@ -121,7 +121,7 @@ class Command(BaseCommand): ) with open(writer.path, "w", encoding="utf-8") as fh: fh.write(migration_file_string) - run_formatters([writer.path]) + run_formatters([writer.path], stderr=self.stderr) if verbosity > 0: self.stdout.write( diff --git a/django/core/management/commands/squashmigrations.py b/django/core/management/commands/squashmigrations.py index 6b5ddeeba5..bb636116a5 100644 --- a/django/core/management/commands/squashmigrations.py +++ b/django/core/management/commands/squashmigrations.py @@ -221,7 +221,7 @@ class Command(BaseCommand): ) with open(writer.path, "w", encoding="utf-8") as fh: fh.write(writer.as_string()) - run_formatters([writer.path]) + run_formatters([writer.path], stderr=self.stderr) if self.verbosity > 0: self.stdout.write( diff --git a/django/core/management/templates.py b/django/core/management/templates.py index 633eed781d..dbaea11200 100644 --- a/django/core/management/templates.py +++ b/django/core/management/templates.py @@ -229,7 +229,7 @@ class TemplateCommand(BaseCommand): else: shutil.rmtree(path_to_remove) - run_formatters([top_dir], **formatter_paths) + run_formatters([top_dir], **formatter_paths, stderr=self.stderr) def handle_template(self, template, subdir): """ diff --git a/django/core/management/utils.py b/django/core/management/utils.py index fca61f2c23..6ca88fa71a 100644 --- a/django/core/management/utils.py +++ b/django/core/management/utils.py @@ -2,6 +2,8 @@ import fnmatch import os import shutil import subprocess +import sys +import traceback from pathlib import Path from subprocess import run @@ -161,7 +163,7 @@ def find_formatters(): return {"black_path": shutil.which("black")} -def run_formatters(written_files, black_path=(sentinel := object())): +def run_formatters(written_files, black_path=(sentinel := object()), stderr=sys.stderr): """ Run the black formatter on the specified files. """ @@ -169,7 +171,11 @@ def run_formatters(written_files, black_path=(sentinel := object())): if black_path is sentinel: black_path = shutil.which("black") if black_path: - subprocess.run( - [black_path, "--fast", "--", *written_files], - capture_output=True, - ) + try: + subprocess.run( + [black_path, "--fast", "--", *written_files], + capture_output=True, + ) + except OSError: + stderr.write("Formatters failed to launch:") + traceback.print_exc(file=stderr) diff --git a/tests/admin_scripts/tests.py b/tests/admin_scripts/tests.py index 6fdd873661..3145723920 100644 --- a/tests/admin_scripts/tests.py +++ b/tests/admin_scripts/tests.py @@ -16,6 +16,8 @@ import unittest from io import StringIO from unittest import mock +from user_commands.utils import AssertFormatterFailureCaughtContext + from django import conf, get_version from django.conf import settings from django.core.management import ( @@ -2943,6 +2945,16 @@ class StartProject(LiveServerTestCase, AdminScriptTestCase): expected_mode, ) + def test_failure_to_format_code(self): + with AssertFormatterFailureCaughtContext(self) as ctx: + call_command( + "startapp", + "mynewapp", + directory=self.test_dir, + stdout=ctx.stdout, + stderr=ctx.stderr, + ) + class StartApp(AdminScriptTestCase): def test_invalid_name(self): diff --git a/tests/migrations/test_base.py b/tests/migrations/test_base.py index cde4063d04..41041f51e8 100644 --- a/tests/migrations/test_base.py +++ b/tests/migrations/test_base.py @@ -4,7 +4,10 @@ import tempfile from contextlib import contextmanager from importlib import import_module +from user_commands.utils import AssertFormatterFailureCaughtContext + from django.apps import apps +from django.core.management import call_command from django.db import connection, connections, migrations, models from django.db.migrations.migration import Migration from django.db.migrations.optimizer import MigrationOptimizer @@ -168,6 +171,15 @@ class MigrationTestBase(TransactionTestCase): def assertFKNotExists(self, table, columns, to): return self.assertFKExists(table, columns, to, False) + def assertFormatterFailureCaught( + self, *args, module="migrations.test_migrations", **kwargs + ): + with ( + self.temporary_migration_module(module=module), + AssertFormatterFailureCaughtContext(self) as ctx, + ): + call_command(*args, stdout=ctx.stdout, stderr=ctx.stderr, **kwargs) + @contextmanager def temporary_migration_module(self, app_label="migrations", module=None): """ diff --git a/tests/migrations/test_commands.py b/tests/migrations/test_commands.py index 724c88a28f..6c3c67fd61 100644 --- a/tests/migrations/test_commands.py +++ b/tests/migrations/test_commands.py @@ -2265,6 +2265,19 @@ class MakeMigrationsTests(MigrationTestBase): self.assertEqual(out.getvalue(), f"{merge_file}\n") self.assertIn(f"Created new merge migration {merge_file}", err.getvalue()) + def test_makemigrations_failure_to_format_code(self): + self.assertFormatterFailureCaught("makemigrations", "migrations") + + def test_merge_makemigrations_failure_to_format_code(self): + self.assertFormatterFailureCaught("makemigrations", "migrations", empty=True) + self.assertFormatterFailureCaught( + "makemigrations", + "migrations", + merge=True, + interactive=False, + module="migrations.test_migrations_conflict", + ) + def test_makemigrations_migrations_modules_path_not_exist(self): """ makemigrations creates migrations when specifying a custom location @@ -3069,6 +3082,11 @@ class SquashMigrationsTests(MigrationTestBase): + black_warning, ) + def test_failure_to_format_code(self): + self.assertFormatterFailureCaught( + "squashmigrations", "migrations", "0002", interactive=False + ) + class AppLabelErrorTests(TestCase): """ @@ -3302,6 +3320,9 @@ class OptimizeMigrationTests(MigrationTestBase): with self.assertRaisesMessage(CommandError, msg): call_command("optimizemigration", "migrations", "nonexistent") + def test_failure_to_format_code(self): + self.assertFormatterFailureCaught("optimizemigration", "migrations", "0001") + class CustomMigrationCommandTests(MigrationTestBase): @override_settings( diff --git a/tests/user_commands/test_files/black b/tests/user_commands/test_files/black new file mode 100644 index 0000000000..d95ebc8c1e --- /dev/null +++ b/tests/user_commands/test_files/black @@ -0,0 +1 @@ +# This file is not executable, to simulate a broken `black` install. diff --git a/tests/user_commands/tests.py b/tests/user_commands/tests.py index 2a1e904f3b..2add272c10 100644 --- a/tests/user_commands/tests.py +++ b/tests/user_commands/tests.py @@ -1,6 +1,8 @@ import os +import sys from argparse import ArgumentDefaultsHelpFormatter from io import StringIO +from pathlib import Path from unittest import mock from admin_scripts.tests import AdminScriptTestCase @@ -15,6 +17,7 @@ from django.core.management.utils import ( is_ignored_path, normalize_path_patterns, popen_wrapper, + run_formatters, ) from django.db import connection from django.test import SimpleTestCase, override_settings @@ -22,6 +25,7 @@ from django.test.utils import captured_stderr, extend_sys_path from django.utils import translation from .management.commands import dance +from .utils import AssertFormatterFailureCaughtContext # A minimal set of apps to avoid system checks running on all apps. @@ -535,3 +539,24 @@ class UtilsTests(SimpleTestCase): def test_normalize_path_patterns_truncates_wildcard_base(self): expected = [os.path.normcase(p) for p in ["foo/bar", "bar/*/"]] self.assertEqual(normalize_path_patterns(["foo/bar/*", "bar/*/"]), expected) + + def test_run_formatters_handles_oserror_for_black_path(self): + cases = [ + (FileNotFoundError, "nonexistent"), + ( + OSError if sys.platform == "win32" else PermissionError, + str(Path(__file__).parent / "test_files" / "black"), + ), + ] + for exception, location in cases: + with ( + self.subTest(exception.__qualname__), + AssertFormatterFailureCaughtContext( + self, shutil_which_result=location + ) as ctx, + ): + run_formatters([], stderr=ctx.stderr) + parsed_error = ctx.stderr.getvalue() + self.assertIn(exception.__qualname__, parsed_error) + if sys.platform != "win32": + self.assertIn(location, parsed_error) diff --git a/tests/user_commands/utils.py b/tests/user_commands/utils.py new file mode 100644 index 0000000000..84f6bdbc5c --- /dev/null +++ b/tests/user_commands/utils.py @@ -0,0 +1,23 @@ +from io import StringIO +from unittest import mock + + +class AssertFormatterFailureCaughtContext: + + def __init__(self, test, shutil_which_result="nonexistent"): + self.stdout = StringIO() + self.stderr = StringIO() + self.test = test + self.shutil_which_result = shutil_which_result + + def __enter__(self): + self.mocker = mock.patch( + "django.core.management.utils.shutil.which", + return_value=self.shutil_which_result, + ) + self.mocker.start() + return self + + def __exit__(self, exc_type, exc_value, traceback): + self.mocker.stop() + self.test.assertIn("Formatters failed to launch", self.stderr.getvalue())