From 666a6ce6035b22ac1589e6e8e195d2cd3436cd9c Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Thu, 10 Oct 2024 20:33:25 +0100 Subject: [PATCH] Add test skip decorator fixer (#498) Start of #364. --- CHANGELOG.rst | 20 +++ README.rst | 40 ++++++ src/django_upgrade/ast.py | 42 ++++++ .../fixers/versioned_branches.py | 42 +----- .../fixers/versioned_test_skip_decorators.py | 60 +++++++++ .../test_versioned_test_skip_decorators.py | 127 ++++++++++++++++++ 6 files changed, 294 insertions(+), 37 deletions(-) create mode 100644 src/django_upgrade/fixers/versioned_test_skip_decorators.py create mode 100644 tests/fixers/test_versioned_test_skip_decorators.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 919f6750..b3f50e02 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -6,6 +6,26 @@ Changelog Thanks to Tobias Funke for the report in `Issue #495 `__. +* Add all-version fixer to remove outdated test skip decorators. + For example: + + .. code-block:: diff + + import django + from django.test import TestCase + + class ExampleTests(TestCase): + - @unittest.skipIf(django.VERSION < (5, 1), "Django 5.1+") + def test_one(self): + ... + + - @unittest.skipUnless(django.VERSION >= (5, 1), "Django 5.1+") + def test_two(self): + ... + + + `Issue #364 `__. + * Drop Python 3.8 support. * Support Python 3.13. diff --git a/README.rst b/README.rst index 5b29ff4b..d47b7837 100644 --- a/README.rst +++ b/README.rst @@ -218,6 +218,46 @@ A single ``else`` block may be present, but ``elif`` is not supported. See also `pyupgrade’s similar feature `__ that removes outdated code from checks on the Python version. +Versioned test skip decorators +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +**Name:** ``versioned_test_skip_decorators`` + +Removes outdated test skip decorators that compare to ``django.VERSION``. +Like the above, it requires comparisons of the form: + +.. code-block:: text + + django.VERSION (, ) + +Supports these test skip decorators: + +* |unittest.skipIf|__: + + .. |unittest.skipIf| replace:: ``@unittest.skipIf`` + __ https://docs.python.org/3/library/unittest.html#unittest.skipIf + +* |unittest.skipUnless|__: + + .. |unittest.skipUnless| replace:: ``@unittest.skipUnless`` + __ https://docs.python.org/3/library/unittest.html#unittest.skipUnless + +For example: + +.. code-block:: diff + + import django + from django.test import TestCase + + class ExampleTests(TestCase): + - @unittest.skipIf(django.VERSION < (5, 1), "Django 5.1+") + def test_one(self): + ... + + - @unittest.skipUnless(django.VERSION >= (5, 1), "Django 5.1+") + def test_two(self): + ... + Django 5.1 ---------- diff --git a/src/django_upgrade/ast.py b/src/django_upgrade/ast.py index de90eab4..df268387 100644 --- a/src/django_upgrade/ast.py +++ b/src/django_upgrade/ast.py @@ -3,9 +3,12 @@ import ast import warnings from typing import Literal +from typing import cast from tokenize_rt import Offset +from django_upgrade.data import State + def ast_parse(contents_text: str) -> ast.Module: # intentionally ignore warnings, we can't do anything about them @@ -50,3 +53,42 @@ def looks_like_test_client_call( and isinstance(node.func.value.value, ast.Name) and node.func.value.value.id == "self" ) + + +def is_passing_comparison( + test: ast.Compare, state: State +) -> Literal["pass", "fail", None]: + """ + Return whether the given ast.Compare node compares a version tuple with + django.VERSION and would pass or fail for the current target version, or + None if no match or cannot determine. + """ + if not ( + isinstance(left := test.left, ast.Attribute) + and isinstance(left.value, ast.Name) + and left.value.id == "django" + and left.attr == "VERSION" + and len(test.ops) == 1 + and isinstance(test.ops[0], (ast.Gt, ast.GtE, ast.Lt, ast.LtE)) + and len(test.comparators) == 1 + and isinstance((comparator := test.comparators[0]), ast.Tuple) + and len(comparator.elts) == 2 + and all(isinstance(e, ast.Constant) for e in comparator.elts) + and all(isinstance(cast(ast.Constant, e).value, int) for e in comparator.elts) + ): + return None + + min_version = tuple(cast(ast.Constant, e).value for e in comparator.elts) + if isinstance(test.ops[0], ast.Gt): + if state.settings.target_version > min_version: + return "pass" + elif isinstance(test.ops[0], ast.GtE): + if state.settings.target_version >= min_version: + return "pass" + elif isinstance(test.ops[0], ast.Lt): + if state.settings.target_version >= min_version: + return "fail" + else: # ast.LtE + if state.settings.target_version > min_version: + return "fail" + return None diff --git a/src/django_upgrade/fixers/versioned_branches.py b/src/django_upgrade/fixers/versioned_branches.py index 39b56533..3adac130 100644 --- a/src/django_upgrade/fixers/versioned_branches.py +++ b/src/django_upgrade/fixers/versioned_branches.py @@ -12,12 +12,12 @@ from collections.abc import Iterable from functools import partial from typing import Literal -from typing import cast from tokenize_rt import Offset from tokenize_rt import Token from django_upgrade.ast import ast_start_offset +from django_upgrade.ast import is_passing_comparison from django_upgrade.data import Fixer from django_upgrade.data import State from django_upgrade.data import TokenFunc @@ -37,11 +37,7 @@ def visit_If( ) -> Iterable[tuple[Offset, TokenFunc]]: if ( isinstance(node.test, ast.Compare) - and isinstance(left := node.test.left, ast.Attribute) - and isinstance(left.value, ast.Name) - and left.value.id == "django" - and left.attr == "VERSION" - and (keep_branch := _is_passing_comparison(node.test, state)) is not None + and (pass_fail := is_passing_comparison(node.test, state)) is not None and ( # do not handle 'if ... elif ...' not node.orelse @@ -49,40 +45,12 @@ def visit_If( ) ): yield ast_start_offset(node), partial( - _fix_block, node=node, keep_branch=keep_branch + _fix_block, + node=node, + keep_branch=("first" if pass_fail == "pass" else "second"), ) -def _is_passing_comparison( - test: ast.Compare, state: State -) -> Literal["first", "second", None]: - if not ( - len(test.ops) == 1 - and isinstance(test.ops[0], (ast.Gt, ast.GtE, ast.Lt, ast.LtE)) - and len(test.comparators) == 1 - and isinstance((comparator := test.comparators[0]), ast.Tuple) - and len(comparator.elts) == 2 - and all(isinstance(e, ast.Constant) for e in comparator.elts) - and all(isinstance(cast(ast.Constant, e).value, int) for e in comparator.elts) - ): - return None - - min_version = tuple(cast(ast.Constant, e).value for e in comparator.elts) - if isinstance(test.ops[0], ast.Gt): - if state.settings.target_version > min_version: - return "first" - elif isinstance(test.ops[0], ast.GtE): - if state.settings.target_version >= min_version: - return "first" - elif isinstance(test.ops[0], ast.Lt): - if state.settings.target_version >= min_version: - return "second" - else: # ast.LtE - if state.settings.target_version > min_version: - return "second" - return None - - def _fix_block( tokens: list[Token], i: int, diff --git a/src/django_upgrade/fixers/versioned_test_skip_decorators.py b/src/django_upgrade/fixers/versioned_test_skip_decorators.py new file mode 100644 index 00000000..e64f24f1 --- /dev/null +++ b/src/django_upgrade/fixers/versioned_test_skip_decorators.py @@ -0,0 +1,60 @@ +from __future__ import annotations + +import ast +from collections.abc import Iterable +from functools import partial + +from tokenize_rt import Offset +from tokenize_rt import Token + +from django_upgrade.ast import ast_start_offset +from django_upgrade.ast import is_passing_comparison +from django_upgrade.data import Fixer +from django_upgrade.data import State +from django_upgrade.data import TokenFunc +from django_upgrade.tokens import OP +from django_upgrade.tokens import erase_node +from django_upgrade.tokens import reverse_find + +fixer = Fixer( + __name__, + min_version=(0, 0), +) + + +@fixer.register(ast.FunctionDef) +def visit_FunctionDef( + state: State, + node: ast.FunctionDef, + parents: tuple[ast.AST, ...], +) -> Iterable[tuple[Offset, TokenFunc]]: + for decorator in node.decorator_list: + if ( + isinstance(decorator, ast.Call) + and isinstance(decorator.func, ast.Attribute) + and isinstance(decorator.func.value, ast.Name) + and decorator.func.value.id == "unittest" + and decorator.func.attr in ("skipIf", "skipUnless") + and len(decorator.args) == 2 + and isinstance(decorator.args[0], ast.Compare) + and ( + (pass_fail := is_passing_comparison(decorator.args[0], state)) + is not None + ) + and ( + (decorator.func.attr == "skipIf" and pass_fail == "fail") + or (decorator.func.attr == "skipUnless" and pass_fail == "pass") + ) + ): + yield ast_start_offset(decorator), partial(erase_decorator, node=decorator) + + +def erase_decorator( + tokens: list[Token], + i: int, + *, + node: ast.Call, +) -> None: + erase_node(tokens, i, node=node) + at_j = reverse_find(tokens, i, name=OP, src="@") + del tokens[at_j:i] diff --git a/tests/fixers/test_versioned_test_skip_decorators.py b/tests/fixers/test_versioned_test_skip_decorators.py new file mode 100644 index 00000000..322f8ac3 --- /dev/null +++ b/tests/fixers/test_versioned_test_skip_decorators.py @@ -0,0 +1,127 @@ +from __future__ import annotations + +from functools import partial + +from django_upgrade.data import Settings +from tests.fixers import tools + +settings = Settings(target_version=(4, 1)) +check_noop = partial(tools.check_noop, settings=settings) +check_transformed = partial(tools.check_transformed, settings=settings) + + +def test_unittest_skip_left(): + check_noop( + """\ + import unittest + + @unittest.skip("Always skipped") + def test_thing(self): + pass + """, + ) + + +def test_unittest_skipIf_too_few_args(): + check_noop( + """\ + import unittest + import django + + @unittest.skipIf(django.VERSION < (4, 1)) + def test_thing(self): + pass + """, + ) + + +def test_unittest_skipIf_too_many_args(): + check_noop( + """\ + import unittest + import django + + @unittest.skipIf(django.VERSION < (4, 1), "Django 4.1+", "what is this arg?") + def test_thing(self): + pass + """, + ) + + +def test_unittest_skipIf_passing_comparison(): + check_noop( + """\ + import unittest + import django + + @unittest.skipIf(django.VERSION < (4, 2), "Django 4.2+") + def test_thing(self): + pass + """, + ) + + +def test_unittest_skipIf_unknown_comparison(): + check_noop( + """\ + import unittest + import django + + @unittest.skipIf(django.VERSION < (4, 1, 1), "Django 4.1.1+") + def test_thing(self): + pass + """, + ) + + +def test_unittest_skipUnless_failing_comparison(): + check_noop( + """\ + import unittest + import django + + @unittest.skipUnless(django.VERSION >= (4, 2), "Django 4.2+") + def test_thing(self): + pass + """, + ) + + +def test_unittest_skipIf_removed(): + check_transformed( + """\ + import unittest + import django + + @unittest.skipIf(django.VERSION < (4, 1), "Django 4.1+") + def test_thing(self): + pass + """, + """\ + import unittest + import django + + def test_thing(self): + pass + """, + ) + + +def test_skipUnless_removed(): + check_transformed( + """\ + import unittest + import django + + @unittest.skipUnless(django.VERSION >= (4, 1), "Django 4.1+") + def test_thing(self): + pass + """, + """\ + import unittest + import django + + def test_thing(self): + pass + """, + )