Repository: MarketSquare/robotframework-robocop (python)

Issue:
Add negative tests for configuring RuleParams in rules

Pull request:
Wrap known errors such as configuration errors
Recreated old behaviour when rising exception for know issues (like invalid type of config) will only print error message, without code stack. It's done using typer.Exit with exit_code = 2. Error messages are printed to stderr.

Closes #1091 -> most of the rule params use standard types, which will raise ValueError. It's enough we catch this exception - I have added tests for line-too-long rule for it.



Patch:
diff --git a/src/robocop/config.py b/src/robocop/config.py
index fc6ec6f..9e67fe3 100644
--- a/src/robocop/config.py
+++ b/src/robocop/config.py
@@ -819,7 +819,7 @@ class ConfigManager:
             if source in self._paths:
                 continue
             if not source.exists():  # TODO only for passed sources
-                raise errors.FileError(source)
+                raise errors.FatalError(f"File '{source}' does not exist")
             config = self.get_config_for_source_file(source)
             if not ignore_file_filters:
                 if config.file_filters.path_excluded(source):
diff --git a/src/robocop/errors.py b/src/robocop/errors.py
index 02b9481..fbc2670 100644
--- a/src/robocop/errors.py
+++ b/src/robocop/errors.py
@@ -1,11 +1,16 @@
-class RobocopFatalError(ValueError):
-    pass
+import typer
+from rich.console import Console
+
 
+class FatalError(typer.Exit):
+    def __init__(self, msg: str):
+        console = Console(stderr=True)
+        console.print(f"[red]{self.__class__.__name__}[/red]: {msg}")
+        super().__init__(code=2)
 
-class FileError(RobocopFatalError):
-    def __init__(self, source):
-        msg = f'File "{source}" does not exist'
-        super().__init__(msg)
+
+class RobocopFatalError(ValueError):
+    pass
 
 
 class InvalidParameterFormatError(RobocopFatalError):
diff --git a/src/robocop/files.py b/src/robocop/files.py
index 1908379..fab3d09 100644
--- a/src/robocop/files.py
+++ b/src/robocop/files.py
@@ -18,9 +18,7 @@ def load_toml_file(config_path: Path) -> dict[str, Any]:
         with config_path.open("rb") as tf:
             return toml.load(tf)
     except (toml.TOMLDecodeError, OSError) as e:
-        raise click.FileError(
-            filename=str(config_path), hint=f"Error reading configuration file: {e}"
-        ) from None  # TODO: check typer errors
+        raise click.FileError(filename=str(config_path), hint=f"Error reading configuration file: {e}") from None
 
 
 def read_toml_config(config_path: Path) -> dict[str, Any] | None:
diff --git a/src/robocop/formatter/exceptions.py b/src/robocop/formatter/exceptions.py
index 840086e..3b4b287 100644
--- a/src/robocop/formatter/exceptions.py
+++ b/src/robocop/formatter/exceptions.py
@@ -1,12 +1,9 @@
 from __future__ import annotations
 
-import sys
+from robocop.errors import FatalError
 
 
-class RobocopConfigError(Exception):
-    def __init__(self, err):
-        print(f"Error: {err}")
-        sys.exit(1)
+class RobocopConfigError(FatalError): ...
 
 
 class InvalidParameterValueError(RobocopConfigError):
@@ -22,12 +19,5 @@ class InvalidParameterError(RobocopConfigError):
         )
 
 
-class InvalidParameterFormatError(RobocopConfigError):
-    def __init__(self, formatter):
-        super().__init__(
-            f"{formatter}: Invalid parameter format. Pass parameters using MyFormatter.param_name=value syntax."
-        )
-
-
 class ImportFormatterError(RobocopConfigError):
     pass
diff --git a/src/robocop/linter/exceptions.py b/src/robocop/linter/exceptions.py
index 169e6ca..1c2266d 100644
--- a/src/robocop/linter/exceptions.py
+++ b/src/robocop/linter/exceptions.py
@@ -5,27 +5,25 @@ if TYPE_CHECKING:
 
 import robot.errors
 
-
-class RobocopFatalError(ValueError):
-    pass
+from robocop.errors import FatalError
 
 
-class ConfigGeneralError(RobocopFatalError):
+class ConfigGeneralError(FatalError):
     pass
 
 
-class InvalidExternalCheckerError(RobocopFatalError):
+class InvalidExternalCheckerError(FatalError):
     def __init__(self, path):
         msg = f'Fatal error: Failed to load external rules from file "{path}". Verify if the file exists'
         super().__init__(msg)
 
 
-class InvalidArgumentError(RobocopFatalError):
+class InvalidArgumentError(FatalError):
     def __init__(self, msg):
         super().__init__(f"Invalid configuration for Robocop:\n{msg}")
 
 
-class RuleNotFoundError(RobocopFatalError):
+class RuleNotFoundError(FatalError):
     def __init__(self, rule, checker):
         super().__init__(
             f"{checker.__class__.__name__} checker does not contain rule `{rule}`. "
@@ -33,7 +31,7 @@ class RuleNotFoundError(RobocopFatalError):
         )
 
 
-class RuleParamNotFoundError(RobocopFatalError):  # TODO
+class RuleParamNotFoundError(FatalError):  # TODO
     def __init__(self, rule, param, checker):
         super().__init__(
             f"Rule `{rule.name}` in `{checker.__class__.__name__}` checker does not contain `{param}` param. "
@@ -41,7 +39,7 @@ class RuleParamNotFoundError(RobocopFatalError):  # TODO
         )
 
 
-class RuleParamFailedInitError(RobocopFatalError):  # TODO
+class RuleParamFailedInitError(FatalError):  # TODO
     def __init__(self, param, value, err):
         desc = f"    Parameter info: {param.desc}" if param.desc else ""
         super().__init__(


Test patch:
diff --git a/tests/linter/cli/test_thresholds.py b/tests/linter/cli/test_thresholds.py
index 1e7987e..1b89b97 100644
--- a/tests/linter/cli/test_thresholds.py
+++ b/tests/linter/cli/test_thresholds.py
@@ -1,3 +1,6 @@
+import os
+from unittest import mock
+
 import pytest
 
 from robocop.config import RuleMatcher
@@ -79,15 +82,23 @@ class TestRuleSeverityThreshold:
             RuleSeverity.ERROR,
         ]
 
-    # # TODO
-    def test_invalid_threshold_config(self):
-        thresholds = SeverityThreshold("line_length")
-        exp_error = (
-            "Invalid configuration for Robocop:\n"
-            "Invalid severity value 'error'. It should be list of `severity=param_value` pairs, separated by `:`."
-        )
-        with pytest.raises(exceptions.InvalidArgumentError, match=exp_error):
-            thresholds.value = "error"
-        exp_error = "Invalid severity value 'invalid'. Choose one from: I, W, E"
-        with pytest.raises(exceptions.InvalidArgumentError, match=exp_error):
-            thresholds.value = "invalid=100"
+    def test_invalid_threshold_config(self, capsys):
+        # By default, pytest uses 80-chars long terminal which depends on COLUMNS env variable
+        with mock.patch.dict(os.environ, {"COLUMNS": "200"}, clear=True):
+            thresholds = SeverityThreshold("line_length")
+            exp_error = (
+                "InvalidArgumentError: Invalid configuration for Robocop:\n"
+                "Invalid severity value 'error'. It should be list of `severity=param_value` pairs, separated by `:`.\n"
+            )
+            with pytest.raises(exceptions.InvalidArgumentError):
+                thresholds.value = "error"
+            _, err = capsys.readouterr()
+            assert err == exp_error
+            exp_error = (
+                "InvalidArgumentError: Invalid configuration for Robocop:\n"
+                "Invalid severity value 'invalid'. Choose one from: I, W, E.\n"
+            )
+            with pytest.raises(exceptions.InvalidArgumentError):
+                thresholds.value = "invalid=100"
+            _, err = capsys.readouterr()
+            assert err == exp_error
diff --git a/tests/linter/reports/test_reports.py b/tests/linter/reports/test_reports.py
index 19cfe0f..6ec3f56 100644
--- a/tests/linter/reports/test_reports.py
+++ b/tests/linter/reports/test_reports.py
@@ -43,10 +43,12 @@ def test_get_reports_all(config):
     assert reports_list.index("version") < reports_list.index("timestamp") < reports_list.index("sarif")
 
 
-def test_get_unknown_report(config):
+def test_get_unknown_report(config, capsys):
     config.linter.reports = ["all", "unknown"]
-    with pytest.raises(robocop.linter.exceptions.InvalidReportName, match="Provided report 'unknown' does not exist."):
+    with pytest.raises(robocop.linter.exceptions.InvalidReportName):
         get_reports(config)
+    _, err = capsys.readouterr()
+    assert err == "InvalidReportName: Provided report 'unknown' does not exist. \n"
 
 
 def clear_cache():
diff --git a/tests/linter/rules/lengths/line_too_long/expected_exception.txt b/tests/linter/rules/lengths/line_too_long/expected_exception.txt
new file mode 100644
index 0000000..a4659fa
--- /dev/null
+++ b/tests/linter/rules/lengths/line_too_long/expected_exception.txt
@@ -0,0 +1,3 @@
+RuleParamFailedInitError: Failed to configure param `line_length` with value `string`. Received error `invalid literal for int() with base 10: 'string'`.
+    Parameter type: <class 'int'>
+    Parameter info: number of characters allowed in line
\ No newline at end of file
diff --git a/tests/linter/rules/lengths/line_too_long/test_rule.py b/tests/linter/rules/lengths/line_too_long/test_rule.py
index a4c1d80..aefa854 100644
--- a/tests/linter/rules/lengths/line_too_long/test_rule.py
+++ b/tests/linter/rules/lengths/line_too_long/test_rule.py
@@ -15,5 +15,7 @@ class TestRuleAcceptance(RuleAcceptance):
             expected_file="expected_output_severity_threshold.txt",
         )
 
-
-# TODO: test with line length
+    def test_configure_invalid_line_length(self):
+        self.check_rule(
+            configure=["line-too-long.line_length=string"], expected_file="expected_exception.txt", exit_code=2
+        )
diff --git a/tests/linter/utils/__init__.py b/tests/linter/utils/__init__.py
index 8ac699e..10fd08f 100644
--- a/tests/linter/utils/__init__.py
+++ b/tests/linter/utils/__init__.py
@@ -96,6 +96,7 @@ class RuleAcceptance:
         language: list[str] | None = None,
         output_format: str = "simple",
         deprecated: bool = False,
+        exit_code: int | None = None,
         **kwargs,
     ):
         if not self.enabled_in_version(test_on_version):
@@ -115,7 +116,7 @@ class RuleAcceptance:
         configure.append(f"print_issues.output_format={output_format}")
         with isolated_output() as output, working_directory(test_data):
             try:
-                with pytest.raises(click.exceptions.Exit):
+                with pytest.raises(click.exceptions.Exit) as excinfo:
                     check_files(
                         sources=paths,
                         select=select,
@@ -130,6 +131,8 @@ class RuleAcceptance:
                 sys.stdout.flush()
                 result = get_result(output)
                 parsed_results = result.splitlines()
+        if exit_code is not None:
+            assert excinfo.value.exit_code == exit_code
         actual = normalize_result(parsed_results, test_data, sort_lines=sort_lines)
         if deprecated:
             assert actual