Improve feedback for fix, avoid a traceback with transform (#4148)

.github/workflows/tox.yml

@@ -73,7 +73,7 @@ jobs:
     env:
       # Number of expected test passes, safety measure for accidental skip of
       # tests. Update value if you add/remove tests.
-      PYTEST_REQPASS: 860
+      PYTEST_REQPASS: 865
     steps:
       - uses: actions/checkout@v4
         with:

.pre-commit-config.yaml

@@ -137,7 +137,7 @@ repos:
         types: [file, yaml]
         entry: yamllint --strict
   - repo: https://github.com/astral-sh/ruff-pre-commit
-    rev: "v0.4.3"
+    rev: "v0.4.4"
     hooks:
       - id: ruff
         args: [--fix, --exit-non-zero-on-fix]

src/ansiblelint/transformer.py

@@ -1,3 +1,4 @@
+# cspell:ignore classinfo
 """Transformer implementation."""
 
 from __future__ import annotations
@@ -33,6 +34,17 @@ class Transformer:
     pre-requisite for the planned rule-specific transforms.
     """
 
+    DUMP_MSG = "Rewriting yaml file:"
+    FIX_NA_MSG = "Rule specific fix not available for:"
+    FIX_NE_MSG = "Rule specific fix not enabled for:"
+    FIX_APPLY_MSG = "Applying rule specific fix for:"
+    FIX_FAILED_MSG = "Rule specific fix failed for:"
+    FIX_ISSUE_MSG = (
+        "Please file an issue for this with the task or playbook that caused the error."
+    )
+    FIX_APPLIED_MSG = "Rule specific fix applied for:"
+    FIX_NOT_APPLIED_MSG = "Rule specific fix not applied for:"
+
     def __init__(self, result: LintResult, options: Options):
         """Initialize a Transformer instance."""
         self.write_set = self.effective_write_set(options.write_list)
@@ -113,7 +125,7 @@ def run(self) -> None:
                 self._do_transforms(file, ruamel_data or data, file_is_yaml, matches)
 
             if file_is_yaml:
-                _logger.debug("Dumping %s using YAML (%s)", file, yaml.version)
+                _logger.debug("%s %s, version=%s", self.DUMP_MSG, file, yaml.version)
                 # noinspection PyUnboundLocalVariable
                 file.content = yaml.dumps(ruamel_data)
 
@@ -129,14 +141,16 @@ def _do_transforms(
     ) -> None:
         """Do Rule-Transforms handling any last-minute MatchError inspections."""
         for match in sorted(matches):
+            match_id = f"{match.tag}/{match.match_type} {match.filename}:{match.lineno}"
             if not isinstance(match.rule, TransformMixin):
+                logging.debug("%s %s", self.FIX_NA_MSG, match_id)
                 continue
             if self.write_set != {"all"}:
                 rule = cast(AnsibleLintRule, match.rule)
                 rule_definition = set(rule.tags)
                 rule_definition.add(rule.id)
                 if rule_definition.isdisjoint(self.write_set):
-                    # rule transform not requested. Skip it.
+                    logging.debug("%s %s", self.FIX_NE_MSG, match_id)
                     continue
             if file_is_yaml and not match.yaml_path:
                 data = cast(CommentedMap | CommentedSeq, data)
@@ -148,4 +162,16 @@ def _do_transforms(
                     "playbook",
                 ):
                     match.yaml_path = get_path_to_task(file, match.lineno, data)
-            match.rule.transform(match, file, data)
+
+            logging.debug("%s %s", self.FIX_APPLY_MSG, match_id)
+            try:
+                match.rule.transform(match, file, data)
+            except Exception as exc:  # pylint: disable=broad-except
+                _logger.error("%s %s", self.FIX_FAILED_MSG, match_id)  # noqa: TRY400
+                _logger.exception(exc)  # noqa: TRY401
+                _logger.error(self.FIX_ISSUE_MSG)  # noqa: TRY400
+                continue
+            if match.fixed:
+                _logger.debug("%s %s", self.FIX_APPLIED_MSG, match_id)
+            else:
+                _logger.error("%s %s", self.FIX_NOT_APPLIED_MSG, match_id)

test/test_transformer.py

@@ -1,25 +1,29 @@
+# cspell:ignore classinfo
 """Tests for Transformer."""
 
 from __future__ import annotations
 
+import builtins
 import os
 import shutil
 from pathlib import Path
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Any
 from unittest import mock
 
 import pytest
 
 import ansiblelint.__main__ as main
 from ansiblelint.app import App
 from ansiblelint.file_utils import Lintable
+from ansiblelint.rules import TransformMixin
 
 # noinspection PyProtectedMember
 from ansiblelint.runner import LintResult, get_matches
 from ansiblelint.transformer import Transformer
 
 if TYPE_CHECKING:
     from ansiblelint.config import Options
+    from ansiblelint.errors import MatchError
     from ansiblelint.rules import RulesCollection
 
 
@@ -354,3 +358,279 @@ def report_outcome(
     main.main()
     assert fix_called
     assert report_called
+
+
+class TransformTests:
+    """A carrier for some common test constants."""
+
+    FILE_NAME = "examples/playbooks/transform-no-free-form.yml"
+    FILE_TYPE = "playbook"
+    LINENO = 5
+    ID = "no-free-form"
+    MATCH_TYPE = "task"
+    VERSION_PART = "version=(1, 1)"
+
+    @classmethod
+    def match_id(cls) -> str:
+        """Generate a match id.
+
+        :returns: Match id string
+        """
+        return f"{cls.ID}/{cls.MATCH_TYPE} {cls.FILE_NAME}:{cls.LINENO}"
+
+    @classmethod
+    def rewrite_part(cls) -> str:
+        """Generate a rewrite part.
+
+        :returns: Rewrite part string
+        """
+        return f"{cls.FILE_NAME} ({cls.FILE_TYPE}), {cls.VERSION_PART}"
+
+
+@pytest.fixture(name="test_result")
+def fixture_test_result(
+    config_options: Options,
+    default_rules_collection: RulesCollection,
+) -> tuple[LintResult, Options]:
+    """Fixture that runs the Runner to populate a LintResult for a given file.
+
+    The results are confirmed and a limited to a single match.
+
+    :param config_options: Configuration options
+    :param default_rules_collection: Default rules collection
+    :returns: Tuple of LintResult and Options
+    """
+    config_options.write_list = [TransformTests.ID]
+    config_options.lintables = [TransformTests.FILE_NAME]
+
+    result = get_matches(rules=default_rules_collection, options=config_options)
+    match = result.matches[0]
+
+    def write(*_args: Any, **_kwargs: Any) -> None:
+        """Don't rewrite the test fixture.
+
+        :param _args: Arguments
+        :param _kwargs: Keyword arguments
+        """
+
+    setattr(match.lintable, "write", write)  # noqa: B010
+
+    assert match.rule.id == TransformTests.ID
+    assert match.filename == TransformTests.FILE_NAME
+    assert match.lineno == TransformTests.LINENO
+    assert match.match_type == TransformTests.MATCH_TYPE
+    result.matches = [match]
+
+    return result, config_options
+
+
+def test_transform_na(
+    caplog: pytest.LogCaptureFixture,
+    monkeypatch: pytest.MonkeyPatch,
+    test_result: tuple[LintResult, Options],
+) -> None:
+    """Test the transformer is not available.
+
+    :param caplog: Log capture fixture
+    :param monkeypatch: Monkeypatch
+    :param test_result: Test result fixture
+    """
+    result = test_result[0]
+    options = test_result[1]
+
+    _isinstance = builtins.isinstance
+    called = False
+
+    def mp_isinstance(t_object: Any, classinfo: type) -> bool:
+        if classinfo is TransformMixin:
+            nonlocal called
+            called = True
+            return False
+        return _isinstance(t_object, classinfo)
+
+    monkeypatch.setattr(builtins, "isinstance", mp_isinstance)
+
+    transformer = Transformer(result=result, options=options)
+    with caplog.at_level(10):
+        transformer.run()
+
+    assert called
+    logs = [record for record in caplog.records if record.module == "transformer"]
+    assert len(logs) == 2
+
+    log_0 = f"{transformer.FIX_NA_MSG} {TransformTests.match_id()}"
+    assert logs[0].message == log_0
+    assert logs[0].levelname == "DEBUG"
+
+    log_1 = f"{transformer.DUMP_MSG} {TransformTests.rewrite_part()}"
+    assert logs[1].message == log_1
+    assert logs[1].levelname == "DEBUG"
+
+
+def test_transform_no_tb(
+    caplog: pytest.LogCaptureFixture,
+    test_result: tuple[LintResult, Options],
+) -> None:
+    """Test the transformer does not traceback.
+
+    :param caplog: Log capture fixture
+    :param test_result: Test result fixture
+    :raises RuntimeError: If the rule is not a TransformMixin
+    """
+    result = test_result[0]
+    options = test_result[1]
+    exception_msg = "FixFailure"
+
+    def transform(*_args: Any, **_kwargs: Any) -> None:
+        """Raise an exception for the transform call.
+
+        :raises RuntimeError: Always
+        """
+        raise RuntimeError(exception_msg)
+
+    if isinstance(result.matches[0].rule, TransformMixin):
+        setattr(result.matches[0].rule, "transform", transform)  # noqa: B010
+    else:
+        err = "Rule is not a TransformMixin"
+        raise TypeError(err)
+
+    transformer = Transformer(result=result, options=options)
+    with caplog.at_level(10):
+        transformer.run()
+
+    logs = [record for record in caplog.records if record.module == "transformer"]
+    assert len(logs) == 5
+
+    log_0 = f"{transformer.FIX_APPLY_MSG} {TransformTests.match_id()}"
+    assert logs[0].message == log_0
+    assert logs[0].levelname == "DEBUG"
+
+    log_1 = f"{transformer.FIX_FAILED_MSG} {TransformTests.match_id()}"
+    assert logs[1].message == log_1
+    assert logs[1].levelname == "ERROR"
+
+    log_2 = exception_msg
+    assert logs[2].message == log_2
+    assert logs[2].levelname == "ERROR"
+
+    log_3 = f"{transformer.FIX_ISSUE_MSG}"
+    assert logs[3].message == log_3
+    assert logs[3].levelname == "ERROR"
+
+    log_4 = f"{transformer.DUMP_MSG} {TransformTests.rewrite_part()}"
+    assert logs[4].message == log_4
+    assert logs[4].levelname == "DEBUG"
+
+
+def test_transform_applied(
+    caplog: pytest.LogCaptureFixture,
+    test_result: tuple[LintResult, Options],
+) -> None:
+    """Test the transformer is applied.
+
+    :param caplog: Log capture fixture
+    :param test_result: Test result fixture
+    """
+    result = test_result[0]
+    options = test_result[1]
+
+    transformer = Transformer(result=result, options=options)
+    with caplog.at_level(10):
+        transformer.run()
+
+    logs = [record for record in caplog.records if record.module == "transformer"]
+    assert len(logs) == 3
+
+    log_0 = f"{transformer.FIX_APPLY_MSG} {TransformTests.match_id()}"
+    assert logs[0].message == log_0
+    assert logs[0].levelname == "DEBUG"
+
+    log_1 = f"{transformer.FIX_APPLIED_MSG} {TransformTests.match_id()}"
+    assert logs[1].message == log_1
+    assert logs[1].levelname == "DEBUG"
+
+    log_2 = f"{transformer.DUMP_MSG} {TransformTests.rewrite_part()}"
+    assert logs[2].message == log_2
+    assert logs[2].levelname == "DEBUG"
+
+
+def test_transform_not_enabled(
+    caplog: pytest.LogCaptureFixture,
+    test_result: tuple[LintResult, Options],
+) -> None:
+    """Test the transformer is not enabled.
+
+    :param caplog: Log capture fixture
+    :param test_result: Test result fixture
+    """
+    result = test_result[0]
+    options = test_result[1]
+    options.write_list = []
+
+    transformer = Transformer(result=result, options=options)
+    with caplog.at_level(10):
+        transformer.run()
+
+    logs = [record for record in caplog.records if record.module == "transformer"]
+    assert len(logs) == 2
+
+    log_0 = f"{transformer.FIX_NE_MSG} {TransformTests.match_id()}"
+    assert logs[0].message == log_0
+    assert logs[0].levelname == "DEBUG"
+
+    log_1 = f"{transformer.DUMP_MSG} {TransformTests.rewrite_part()}"
+    assert logs[1].message == log_1
+    assert logs[1].levelname == "DEBUG"
+
+
+def test_transform_not_applied(
+    caplog: pytest.LogCaptureFixture,
+    test_result: tuple[LintResult, Options],
+) -> None:
+    """Test the transformer is not applied.
+
+    :param caplog: Log capture fixture
+    :param test_result: Test result fixture
+    :raises RuntimeError: If the rule is not a TransformMixin
+    """
+    result = test_result[0]
+    options = test_result[1]
+
+    called = False
+
+    def transform(match: MatchError, *_args: Any, **_kwargs: Any) -> None:
+        """Do not apply the transform.
+
+        :param match: Match object
+        :param _args: Arguments
+        :param _kwargs: Keyword arguments
+        """
+        nonlocal called
+        called = True
+        match.fixed = False
+
+    if isinstance(result.matches[0].rule, TransformMixin):
+        setattr(result.matches[0].rule, "transform", transform)  # noqa: B010
+    else:
+        err = "Rule is not a TransformMixin"
+        raise TypeError(err)
+
+    transformer = Transformer(result=result, options=options)
+    with caplog.at_level(10):
+        transformer.run()
+
+    assert called
+    logs = [record for record in caplog.records if record.module == "transformer"]
+    assert len(logs) == 3
+
+    log_0 = f"{transformer.FIX_APPLY_MSG} {TransformTests.match_id()}"
+    assert logs[0].message == log_0
+    assert logs[0].levelname == "DEBUG"
+
+    log_1 = f"{transformer.FIX_NOT_APPLIED_MSG} {TransformTests.match_id()}"
+    assert logs[1].message == log_1
+    assert logs[1].levelname == "ERROR"
+
+    log_2 = f"{transformer.DUMP_MSG} {TransformTests.rewrite_part()}"
+    assert logs[2].message == log_2
+    assert logs[2].levelname == "DEBUG"