Public Access
1023 lines
39 KiB
Python
1023 lines
39 KiB
Python
"""Unit tests for scripts/ci/pr_review.py."""
|
|
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
import pytest
|
|
from click.testing import CliRunner
|
|
|
|
from devx.ci.pr_review import (
|
|
ReviewResult,
|
|
build_review_body,
|
|
check_architecture_compliance,
|
|
check_best_practices,
|
|
check_commit_conventions,
|
|
check_documentation,
|
|
check_function_length,
|
|
check_i18n,
|
|
check_resource_management,
|
|
check_security,
|
|
check_test_coverage,
|
|
is_python_file,
|
|
is_workflow_only,
|
|
main,
|
|
post_review,
|
|
run_review,
|
|
)
|
|
from devx.exceptions import APIError
|
|
|
|
|
|
class TestIsPythonFile:
|
|
def test_python_file_in_src(self) -> None:
|
|
assert is_python_file("src/devx/cli.py") is True
|
|
|
|
def test_python_file_in_scripts(self) -> None:
|
|
assert is_python_file("scripts/ci/release.py") is True
|
|
|
|
def test_test_file_excluded(self) -> None:
|
|
assert is_python_file("tests/unit/test_cli.py") is False
|
|
|
|
def test_non_python_file(self) -> None:
|
|
assert is_python_file("README.md") is False
|
|
|
|
def test_yaml_file(self) -> None:
|
|
assert is_python_file(".gitea/workflows/ci.yml") is False
|
|
|
|
|
|
class TestIsWorkflowOnly:
|
|
def test_yaml_is_workflow(self) -> None:
|
|
assert is_workflow_only(".gitea/workflows/ci.yml") is True
|
|
|
|
def test_md_is_workflow(self) -> None:
|
|
assert is_workflow_only("README.md") is True
|
|
|
|
def test_python_is_not_workflow(self) -> None:
|
|
assert is_workflow_only("src/devx/cli.py") is False
|
|
|
|
def test_ansible_is_workflow(self) -> None:
|
|
assert is_workflow_only("ansible/tasks/main.yml") is True
|
|
|
|
|
|
class TestReviewResult:
|
|
def test_empty_result_has_no_issues(self) -> None:
|
|
result = ReviewResult()
|
|
assert result.has_issues is False
|
|
|
|
def test_add_issue_makes_has_issues_true(self) -> None:
|
|
result = ReviewResult()
|
|
result.add_issue("src/foo.py", 10, "bad code")
|
|
assert result.has_issues is True
|
|
assert len(result.issues) == 1
|
|
assert result.issues[0]["path"] == "src/foo.py"
|
|
assert result.issues[0]["new_position"] == 10
|
|
|
|
def test_add_summary(self) -> None:
|
|
result = ReviewResult()
|
|
result.add_summary("all good")
|
|
assert "all good" in result.summary
|
|
|
|
|
|
class TestCheckArchitectureCompliance:
|
|
def test_subprocess_in_cli_triggers_issue(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ subprocess.run(['ls'])\n",
|
|
}
|
|
]
|
|
check_architecture_compliance(files, result)
|
|
assert result.has_issues
|
|
assert "subprocess" in result.issues[0]["body"].lower()
|
|
|
|
def test_subprocess_in_other_file_ok(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/executor.py",
|
|
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ subprocess.run(['ls'])\n",
|
|
}
|
|
]
|
|
check_architecture_compliance(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_no_changes_adds_ok_summary(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": ""}]
|
|
check_architecture_compliance(files, result)
|
|
assert any("Architecture compliance: OK" in s for s in result.summary)
|
|
|
|
def test_non_python_file_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "README.md", "patch": "@@ -1,1 +1,2 @@\n+subprocess.run(['ls'])\n"}]
|
|
check_architecture_compliance(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_empty_patch_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": ""}]
|
|
check_architecture_compliance(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_os_system_in_cli_triggers_issue(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ os.system('ls')\n",
|
|
}
|
|
]
|
|
check_architecture_compliance(files, result)
|
|
assert result.has_issues
|
|
assert "os.system" in result.issues[0]["body"]
|
|
|
|
def test_malformed_hunk_header_no_line_number(self) -> None:
|
|
"""A @@ header without a +N line number is handled gracefully."""
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": "@@ -1,2 @@\n+ subprocess.run(['ls'])\n",
|
|
}
|
|
]
|
|
check_architecture_compliance(files, result)
|
|
assert result.has_issues
|
|
|
|
|
|
class TestCheckBestPractices:
|
|
def test_print_triggers_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ print('hello')\n",
|
|
}
|
|
]
|
|
check_best_practices(files, result)
|
|
assert result.has_issues
|
|
assert "print()" in result.issues[0]["body"]
|
|
|
|
def test_bare_except_triggers_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/runner_manager.py",
|
|
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ except:\n pass\n",
|
|
}
|
|
]
|
|
check_best_practices(files, result)
|
|
assert result.has_issues
|
|
assert "bare except" in result.issues[0]["body"]
|
|
|
|
def test_todo_triggers_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ # TODO: fix this\n",
|
|
}
|
|
]
|
|
check_best_practices(files, result)
|
|
assert result.has_issues
|
|
assert "TODO" in result.issues[0]["body"]
|
|
|
|
def test_clean_code_no_issues(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ click.echo('hello')\n",
|
|
}
|
|
]
|
|
check_best_practices(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_empty_patch_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": ""}]
|
|
check_best_practices(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_non_python_file_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "README.md", "patch": "@@ -1,1 +1,2 @@\n+print('hello')\n"}]
|
|
check_best_practices(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_malformed_hunk_header_no_line_number(self) -> None:
|
|
"""A @@ header without a +N line number is handled gracefully."""
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": "@@ -1,2 @@\n+ print('hello')\n",
|
|
}
|
|
]
|
|
check_best_practices(files, result)
|
|
assert result.has_issues
|
|
assert "print()" in result.issues[0]["body"]
|
|
|
|
|
|
class TestCheckSecurity:
|
|
def test_hardcoded_secret_triggers_error(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/config.py",
|
|
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ token = 'abc123secrettoken456'\n",
|
|
}
|
|
]
|
|
check_security(files, result)
|
|
assert result.has_issues
|
|
assert "secret" in result.issues[0]["body"].lower()
|
|
|
|
def test_example_token_not_flagged(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": ".env.example",
|
|
"patch": "@@ -1,1 +1,2 @@\n+token = your-example-token\n",
|
|
}
|
|
]
|
|
check_security(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_shell_true_triggers_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/executor.py",
|
|
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ subprocess.run('ls', shell=True)\n",
|
|
}
|
|
]
|
|
check_best_practices(files, result)
|
|
assert result.has_issues
|
|
assert "shell=True" in result.issues[0]["body"]
|
|
|
|
def test_empty_patch_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/config.py", "patch": ""}]
|
|
check_security(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_non_python_file_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "docs/config.md", "patch": "@@ -1,1 +1,2 @@\n+token = 'abc123secrettoken456'\n"}]
|
|
check_security(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_malformed_hunk_header_no_line_number(self) -> None:
|
|
"""A @@ header without a +N line number is handled gracefully."""
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/config.py",
|
|
"patch": "@@ -1,2 @@\n+ token = 'abc123secrettoken456'\n",
|
|
}
|
|
]
|
|
check_security(files, result)
|
|
assert result.has_issues
|
|
assert "secret" in result.issues[0]["body"].lower()
|
|
|
|
|
|
class TestCheckI18n:
|
|
def test_raw_string_in_echo_triggers_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo("Hello world")\n'}]
|
|
check_i18n(files, result)
|
|
assert result.has_issues
|
|
assert any("i18n" in i["body"] for i in result.issues)
|
|
|
|
def test_translated_string_no_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo(_("Hello world"))\n'}]
|
|
check_i18n(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_fstring_in_echo_triggers_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo(f"Hello {name}")\n'}]
|
|
check_i18n(files, result)
|
|
assert result.has_issues
|
|
|
|
def test_raw_exception_triggers_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": '@@ -1,1 +1,1 @@\n+raise click.ClickException("Something went wrong")\n',
|
|
}
|
|
]
|
|
check_i18n(files, result)
|
|
assert result.has_issues
|
|
|
|
def test_non_src_file_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "scripts/ci/test.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo("Hello world")\n'}]
|
|
check_i18n(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_comment_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": '@@ -1,1 +1,1 @@\n+# click.echo("Hello world")\n'}]
|
|
check_i18n(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_empty_patch_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": ""}]
|
|
check_i18n(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_clean_code_adds_ok_summary(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo(_("Hello world"))\n'}]
|
|
check_i18n(files, result)
|
|
assert any("i18n: OK" in s for s in result.summary)
|
|
|
|
def test_malformed_hunk_header_no_line_number(self) -> None:
|
|
"""A @@ header without a +N line number is handled gracefully."""
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": '@@ -1,2 @@\n+click.echo("Hello world")\n'}]
|
|
check_i18n(files, result)
|
|
assert result.has_issues
|
|
assert any("i18n" in i["body"] for i in result.issues)
|
|
|
|
|
|
class TestCheckResourceManagement:
|
|
def test_open_without_with_triggers_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": '@@ -1,1 +1,1 @@\n+f = open("file.txt")\n'}]
|
|
check_resource_management(files, result)
|
|
assert result.has_issues
|
|
assert any("resource" in i["body"].lower() for i in result.issues)
|
|
|
|
def test_open_with_with_no_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": '@@ -1,1 +1,1 @@\n+with open("file.txt") as f:\n+ pass\n',
|
|
}
|
|
]
|
|
check_resource_management(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_popen_without_cleanup_triggers_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/executor.py",
|
|
"patch": '@@ -1,1 +1,1 @@\n+proc = subprocess.Popen(["cmd"])\n',
|
|
}
|
|
]
|
|
check_resource_management(files, result)
|
|
assert result.has_issues
|
|
|
|
def test_popen_with_communicate_no_warning(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/executor.py",
|
|
"patch": '@@ -1,1 +1,1 @@\n+out, err = subprocess.Popen(["cmd"], stdout=PIPE).communicate()\n',
|
|
}
|
|
]
|
|
check_resource_management(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_comment_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": '@@ -1,1 +1,1 @@\n+# f = open("file.txt")\n'}]
|
|
check_resource_management(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_empty_patch_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": ""}]
|
|
check_resource_management(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_non_python_file_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "docs/config.md", "patch": '@@ -1,1 +1,1 @@\n+f = open("file.txt")\n'}]
|
|
check_resource_management(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_clean_code_adds_ok_summary(self) -> None:
|
|
result = ReviewResult()
|
|
files = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": '@@ -1,1 +1,1 @@\n+with open("file.txt") as f:\n+ data = f.read()\n',
|
|
}
|
|
]
|
|
check_resource_management(files, result)
|
|
assert any("Resource management: OK" in s for s in result.summary)
|
|
|
|
def test_malformed_hunk_header_no_line_number(self) -> None:
|
|
"""A @@ header without a +N line number is handled gracefully."""
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": '@@ -1,2 @@\n+f = open("file.txt")\n'}]
|
|
check_resource_management(files, result)
|
|
assert result.has_issues
|
|
assert any("resource" in i["body"].lower() for i in result.issues)
|
|
|
|
|
|
class TestCheckFunctionLength:
|
|
def test_long_function_triggers_warning(self) -> None:
|
|
result = ReviewResult()
|
|
# Create a patch with a function that adds > 50 lines
|
|
added_lines = "\n".join(f"+ x = {i}" for i in range(55))
|
|
patch = f"@@ -10,3 +10,59 @@\n+def foo():\n+ pass\n{added_lines}\n"
|
|
files = [{"filename": "src/devx/cli.py", "patch": patch}]
|
|
check_function_length(files, result)
|
|
assert result.has_issues
|
|
assert "foo" in result.issues[0]["body"]
|
|
|
|
def test_short_function_no_warning(self) -> None:
|
|
result = ReviewResult()
|
|
patch = "@@ -10,3 +10,8 @@\n def foo():\n pass\n+ x = 1\n+ y = 2\n+ z = 3\n"
|
|
files = [{"filename": "src/devx/cli.py", "patch": patch}]
|
|
check_function_length(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_empty_patch_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": ""}]
|
|
check_function_length(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_non_python_file_skipped(self) -> None:
|
|
result = ReviewResult()
|
|
added_lines = "\n".join(f"+ x = {i}" for i in range(55))
|
|
patch = f"@@ -10,3 +10,59 @@\n+def foo():\n+ pass\n{added_lines}\n"
|
|
files = [{"filename": "README.md", "patch": patch}]
|
|
check_function_length(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_multiple_functions_resets_count(self) -> None:
|
|
"""Two short functions back-to-back should not trigger the length warning."""
|
|
result = ReviewResult()
|
|
patch = "@@ -10,3 +10,15 @@\n+def foo():\n+ x = 1\n+def bar():\n+ y = 2\n"
|
|
files = [{"filename": "src/devx/cli.py", "patch": patch}]
|
|
check_function_length(files, result)
|
|
assert not result.has_issues
|
|
|
|
def test_long_function_followed_by_new_hunk(self) -> None:
|
|
"""Long function followed by @@ header triggers the warning at hunk boundary."""
|
|
result = ReviewResult()
|
|
added_lines = "\n".join(f"+ x = {i}" for i in range(55))
|
|
patch = (
|
|
f"@@ -10,3 +10,59 @@\n+def foo():\n+ pass\n{added_lines}\n@@ -100,3 +100,5 @@\n+def bar():\n+ pass\n"
|
|
)
|
|
files = [{"filename": "src/devx/cli.py", "patch": patch}]
|
|
check_function_length(files, result)
|
|
assert result.has_issues
|
|
assert "foo" in result.issues[0]["body"]
|
|
|
|
def test_long_function_followed_by_new_def(self) -> None:
|
|
"""Long function followed by another def triggers the warning at def boundary."""
|
|
result = ReviewResult()
|
|
added_lines = "\n".join(f"+ x = {i}" for i in range(55))
|
|
patch = f"@@ -10,3 +10,60 @@\n+def foo():\n+ pass\n{added_lines}\n+def bar():\n+ pass\n"
|
|
files = [{"filename": "src/devx/cli.py", "patch": patch}]
|
|
check_function_length(files, result)
|
|
assert result.has_issues
|
|
assert "foo" in result.issues[0]["body"]
|
|
|
|
def test_malformed_hunk_header_no_line_number(self) -> None:
|
|
"""A @@ header without a +N line number is handled gracefully."""
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py", "patch": "@@ -1,2 @@\n+def foo():\n+ pass\n"}]
|
|
check_function_length(files, result)
|
|
assert not result.has_issues
|
|
|
|
|
|
class TestCheckDocumentation:
|
|
def test_src_changes_without_docs_warns(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py"}]
|
|
check_documentation(files, result)
|
|
assert any("WARNING" in s for s in result.summary)
|
|
|
|
def test_src_changes_with_docs_ok(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py"}, {"filename": "docs/user/cli-commands.md"}]
|
|
check_documentation(files, result)
|
|
assert any("Documentation: OK" in s for s in result.summary)
|
|
|
|
def test_ansible_changes_without_docs_warns(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "ansible/roles/gitea-runner/tasks/main.yml"}]
|
|
check_documentation(files, result)
|
|
assert any("WARNING" in s for s in result.summary)
|
|
|
|
def test_only_doc_changes_ok(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "README.md"}]
|
|
check_documentation(files, result)
|
|
assert any("Documentation: OK" in s for s in result.summary)
|
|
|
|
def test_tofu_changes_without_docs_warns(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "tofu/modules/hetzner-vm/main.tf"}]
|
|
check_documentation(files, result)
|
|
assert any("WARNING" in s for s in result.summary)
|
|
|
|
def test_workflow_changes_info(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": ".gitea/workflows/ci.yml"}]
|
|
check_documentation(files, result)
|
|
assert any("INFO" in s for s in result.summary)
|
|
|
|
def test_todo_in_doc_patch_warns(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "docs/guide.md", "patch": "+TODO: fix this later\n+Some content\n"}]
|
|
check_documentation(files, result)
|
|
assert any("TODO" in s for s in result.summary)
|
|
|
|
def test_todo_in_readme_patch_warns(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "README.md", "patch": "+FIXME: broken\n"}]
|
|
check_documentation(files, result)
|
|
assert any("FIXME" in s for s in result.summary)
|
|
|
|
def test_no_todo_in_doc_patch_ok(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "docs/guide.md", "patch": "+Some content\n"}]
|
|
check_documentation(files, result)
|
|
assert not any("TODO" in s for s in result.summary)
|
|
|
|
|
|
class TestCheckTestCoverage:
|
|
def test_src_changes_without_tests_warns(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py"}]
|
|
check_test_coverage(files, result)
|
|
assert any("WARNING" in s for s in result.summary)
|
|
|
|
def test_src_changes_with_tests_ok(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "src/devx/cli.py"}, {"filename": "tests/unit/test_cli.py"}]
|
|
check_test_coverage(files, result)
|
|
assert any("Tests: OK" in s for s in result.summary)
|
|
|
|
def test_only_test_changes_ok(self) -> None:
|
|
result = ReviewResult()
|
|
files = [{"filename": "tests/unit/test_cli.py"}]
|
|
check_test_coverage(files, result)
|
|
assert any("Tests: OK" in s for s in result.summary)
|
|
|
|
|
|
class TestBuildReviewBody:
|
|
def test_body_contains_summary(self) -> None:
|
|
result = ReviewResult()
|
|
result.add_summary("- Architecture compliance: OK")
|
|
body = build_review_body(result)
|
|
assert "Architecture compliance: OK" in body
|
|
assert "Automated PR Review" in body
|
|
|
|
def test_body_contains_issues(self) -> None:
|
|
result = ReviewResult()
|
|
result.add_issue("src/foo.py", 10, "bad code")
|
|
body = build_review_body(result)
|
|
assert "1 issue(s) found" in body
|
|
assert "src/foo.py:10" in body
|
|
assert "bad code" in body
|
|
|
|
def test_body_contains_no_issues_message(self) -> None:
|
|
result = ReviewResult()
|
|
body = build_review_body(result)
|
|
assert "No issues found" in body
|
|
|
|
def test_body_contains_auto_merge_note(self) -> None:
|
|
"""Review body must mention auto-merge."""
|
|
result = ReviewResult()
|
|
body = build_review_body(result)
|
|
assert "Auto-merge" in body
|
|
|
|
|
|
class TestRunReview:
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_run_review_with_no_files(self, mock_client_class: MagicMock) -> None:
|
|
mock_client = mock_client_class.return_value
|
|
mock_client.get_pr_files.return_value = []
|
|
result = run_review(mock_client, "42")
|
|
assert "No files changed" in result.summary[0]
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_run_review_finds_issues(self, mock_client_class: MagicMock) -> None:
|
|
mock_client = mock_client_class.return_value
|
|
mock_client.get_pr_files.return_value = [
|
|
{
|
|
"filename": "src/devx/cli.py",
|
|
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ print('hello')\n",
|
|
}
|
|
]
|
|
mock_client.get_pr_commits.return_value = [{"commit": {"message": "fix: resolve print issue"}}]
|
|
result = run_review(mock_client, "42")
|
|
assert result.has_issues
|
|
|
|
def test_run_review_handles_api_error(self) -> None:
|
|
client = MagicMock()
|
|
client.get_pr_files.side_effect = APIError(404, "Not found")
|
|
result = run_review(client, "42")
|
|
assert any("ERROR" in s for s in result.summary)
|
|
|
|
|
|
class TestCheckCommitConventions:
|
|
def test_conventional_commit_found(self) -> None:
|
|
"""Should report OK when at least one commit is conventional."""
|
|
client = MagicMock()
|
|
client.get_pr_commits.return_value = [
|
|
{"commit": {"message": "fix: resolve bug\n\nDetails"}},
|
|
{"commit": {"message": "wip: testing"}},
|
|
]
|
|
result = ReviewResult()
|
|
check_commit_conventions(client, "42", result)
|
|
assert any("OK" in s for s in result.summary)
|
|
|
|
def test_no_conventional_commit(self) -> None:
|
|
"""Should warn when no commits are conventional."""
|
|
client = MagicMock()
|
|
client.get_pr_commits.return_value = [
|
|
{"commit": {"message": "updated stuff"}},
|
|
{"commit": {"message": "wip: testing"}},
|
|
]
|
|
result = ReviewResult()
|
|
check_commit_conventions(client, "42", result)
|
|
assert any("WARNING" in s for s in result.summary)
|
|
|
|
def test_merge_commits_excluded(self) -> None:
|
|
"""Merge commits should be excluded from the check."""
|
|
client = MagicMock()
|
|
client.get_pr_commits.return_value = [
|
|
{"commit": {"message": "Merge branch 'feature' into master"}},
|
|
{"commit": {"message": "fix: resolve bug"}},
|
|
]
|
|
result = ReviewResult()
|
|
check_commit_conventions(client, "42", result)
|
|
assert any("OK" in s for s in result.summary)
|
|
|
|
def test_all_merges_and_reverts(self) -> None:
|
|
"""Should report OK when all commits are merges/reverts."""
|
|
client = MagicMock()
|
|
client.get_pr_commits.return_value = [
|
|
{"commit": {"message": "Merge branch 'feature' into master"}},
|
|
{"commit": {"message": "Revert: bad commit"}},
|
|
]
|
|
result = ReviewResult()
|
|
check_commit_conventions(client, "42", result)
|
|
assert any("merges/reverts" in s for s in result.summary)
|
|
|
|
def test_no_commits(self) -> None:
|
|
"""Should report OK when there are no commits."""
|
|
client = MagicMock()
|
|
client.get_pr_commits.return_value = []
|
|
result = ReviewResult()
|
|
check_commit_conventions(client, "42", result)
|
|
assert any("no commits" in s for s in result.summary)
|
|
|
|
def test_api_error(self) -> None:
|
|
"""Should report ERROR when API call fails."""
|
|
client = MagicMock()
|
|
client.get_pr_commits.side_effect = APIError(500, "server error")
|
|
result = ReviewResult()
|
|
check_commit_conventions(client, "42", result)
|
|
assert any("ERROR" in s for s in result.summary)
|
|
|
|
|
|
class TestPostReview:
|
|
def test_post_review_with_issues(self) -> None:
|
|
client = MagicMock()
|
|
result = ReviewResult()
|
|
result.add_issue("src/foo.py", 10, "bad code")
|
|
post_review(client, "42", result)
|
|
client.create_review.assert_called_once()
|
|
call_args = client.create_review.call_args
|
|
assert call_args[1]["event"] == "REQUEST_CHANGES"
|
|
assert call_args[1]["comments"] == result.issues
|
|
|
|
def test_post_review_without_issues_uses_comment_not_approve(self) -> None:
|
|
"""Automated review posts COMMENT, not APPROVE (self-approval not allowed)."""
|
|
client = MagicMock()
|
|
result = ReviewResult()
|
|
post_review(client, "42", result)
|
|
client.create_review.assert_called_once()
|
|
call_args = client.create_review.call_args
|
|
assert call_args[1]["event"] == "COMMENT"
|
|
assert call_args[1]["comments"] == []
|
|
|
|
|
|
class TestMain:
|
|
@patch("devx.ci.pr_review.run_review")
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_dry_run_does_not_post(self, mock_client_class: MagicMock, mock_run: MagicMock) -> None:
|
|
mock_run.return_value = ReviewResult()
|
|
runner = CliRunner()
|
|
result = runner.invoke(main, ["42", "my-org/my-repo", "--dry-run"], env={"CI_GITEA_TOKEN": "fake"})
|
|
assert result.exit_code == 0
|
|
assert "[dry-run]" in result.output
|
|
mock_client_class.return_value.create_review.assert_not_called()
|
|
|
|
@patch("devx.ci.pr_review.run_review")
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_post_review_on_success(self, mock_client_class: MagicMock, mock_run: MagicMock) -> None:
|
|
mock_run.return_value = ReviewResult()
|
|
mock_client_class.return_value.create_review.return_value = {"id": 123}
|
|
runner = CliRunner()
|
|
result = runner.invoke(main, ["42", "my-org/my-repo"], env={"CI_GITEA_TOKEN": "fake"})
|
|
assert result.exit_code == 0
|
|
assert "Review #123" in result.output
|
|
mock_client_class.return_value.create_review.assert_called_once()
|
|
|
|
@patch("devx.ci.pr_review.run_review")
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_self_approval_falls_back_to_comment(self, mock_client_class: MagicMock, mock_run: MagicMock) -> None:
|
|
"""If REQUEST_CHANGES fails with 422 (self-approval), fall back to COMMENT."""
|
|
mock_run.return_value = ReviewResult()
|
|
client = mock_client_class.return_value
|
|
client.create_review.side_effect = [
|
|
APIError(422, "approve your own pull is not allowed"),
|
|
{"id": 124},
|
|
]
|
|
runner = CliRunner()
|
|
result = runner.invoke(main, ["42", "my-org/my-repo"], env={"CI_GITEA_TOKEN": "fake"})
|
|
assert result.exit_code == 0
|
|
assert "Review #124" in result.output
|
|
assert client.create_review.call_count == 2
|
|
|
|
@patch("devx.ci.pr_review.run_review")
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_other_api_error_re_raises(self, mock_client_class: MagicMock, mock_run: MagicMock) -> None:
|
|
"""Non-approval API errors should re-raise, not fall back."""
|
|
mock_run.return_value = ReviewResult()
|
|
client = mock_client_class.return_value
|
|
client.create_review.side_effect = APIError(500, "Internal server error")
|
|
runner = CliRunner()
|
|
result = runner.invoke(main, ["42", "my-org/my-repo"], env={"CI_GITEA_TOKEN": "fake"})
|
|
assert result.exit_code != 0
|
|
|
|
@patch.dict("os.environ", {"CI_GITEA_API_TOKEN": "", "CI_GITEA_TOKEN": ""})
|
|
def test_no_token_raises(self) -> None:
|
|
runner = CliRunner()
|
|
result = runner.invoke(main, ["42", "my-org/my-repo"], env={"CI_GITEA_API_TOKEN": "", "CI_GITEA_TOKEN": ""})
|
|
assert result.exit_code != 0
|
|
assert "CI_GITEA_TOKEN" in result.output
|
|
|
|
|
|
class TestManualReview:
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_approve_success(self, mock_client_class: MagicMock) -> None:
|
|
mock_client_class.return_value.create_review.return_value = {"id": 200}
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
[
|
|
"42",
|
|
"oblachno-oss/devx",
|
|
"--event",
|
|
"APPROVE",
|
|
"--body",
|
|
"All 13 REVIEW_CHECKLIST.md categories verified. Architecture: clean. Security: no issues.",
|
|
"--checklist-confirmed",
|
|
"--checklist-categories",
|
|
"1,2,3,4,5,6,7,8,9,10,11,12,13",
|
|
],
|
|
env={"CI_GITEA_TOKEN": "fake"},
|
|
)
|
|
assert result.exit_code == 0
|
|
assert "Review #200" in result.output
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_approve_without_checklist_confirmed_fails(self, mock_client_class: MagicMock) -> None:
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
[
|
|
"42",
|
|
"oblachno-oss/devx",
|
|
"--event",
|
|
"APPROVE",
|
|
"--body",
|
|
"x" * 60,
|
|
"--checklist-categories",
|
|
"1,2,3,4,5,6,7,8",
|
|
],
|
|
env={"CI_GITEA_TOKEN": "fake"},
|
|
)
|
|
assert result.exit_code != 0
|
|
assert "checklist-confirmed" in result.output
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_approve_with_too_few_categories_fails(self, mock_client_class: MagicMock) -> None:
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
[
|
|
"42",
|
|
"oblachno-oss/devx",
|
|
"--event",
|
|
"APPROVE",
|
|
"--body",
|
|
"x" * 60,
|
|
"--checklist-confirmed",
|
|
"--checklist-categories",
|
|
"1,2,3",
|
|
],
|
|
env={"CI_GITEA_TOKEN": "fake"},
|
|
)
|
|
assert result.exit_code != 0
|
|
assert "at least 8" in result.output
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_approve_with_short_body_fails(self, mock_client_class: MagicMock) -> None:
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
[
|
|
"42",
|
|
"oblachno-oss/devx",
|
|
"--event",
|
|
"APPROVE",
|
|
"--body",
|
|
"LGTM",
|
|
"--checklist-confirmed",
|
|
"--checklist-categories",
|
|
"1,2,3,4,5,6,7,8",
|
|
],
|
|
env={"CI_GITEA_TOKEN": "fake"},
|
|
)
|
|
assert result.exit_code != 0
|
|
assert "50 characters" in result.output
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_approve_with_invalid_category_fails(self, mock_client_class: MagicMock) -> None:
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
[
|
|
"42",
|
|
"oblachno-oss/devx",
|
|
"--event",
|
|
"APPROVE",
|
|
"--body",
|
|
"x" * 60,
|
|
"--checklist-confirmed",
|
|
"--checklist-categories",
|
|
"1,2,abc,4",
|
|
],
|
|
env={"CI_GITEA_TOKEN": "fake"},
|
|
)
|
|
assert result.exit_code != 0
|
|
assert "Invalid" in result.output
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_request_changes_success(self, mock_client_class: MagicMock) -> None:
|
|
mock_client_class.return_value.create_review.return_value = {"id": 201}
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
[
|
|
"42",
|
|
"oblachno-oss/devx",
|
|
"--event",
|
|
"REQUEST_CHANGES",
|
|
"--body",
|
|
"Please fix the architecture issues in the CLI module before merging.",
|
|
],
|
|
env={"CI_GITEA_TOKEN": "fake"},
|
|
)
|
|
assert result.exit_code == 0
|
|
assert "Review #201" in result.output
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_manual_review_dry_run(self, mock_client_class: MagicMock) -> None:
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
["42", "oblachno-oss/devx", "--event", "COMMENT", "--body", "x" * 60, "--dry-run"],
|
|
env={"CI_GITEA_TOKEN": "fake"},
|
|
)
|
|
assert result.exit_code == 0
|
|
assert "[dry-run]" in result.output
|
|
mock_client_class.return_value.create_review.assert_not_called()
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_manual_review_self_approval_fallback_to_comment(
|
|
self, mock_client_class: MagicMock, monkeypatch: pytest.MonkeyPatch
|
|
) -> None:
|
|
"""Self-approval with no CI token available → fall back to COMMENT."""
|
|
monkeypatch.delenv("CI_GITEA_API_TOKEN", raising=False)
|
|
monkeypatch.delenv("CI_GITEA_TOKEN", raising=False)
|
|
client = mock_client_class.return_value
|
|
client.create_review.side_effect = [
|
|
APIError(422, "approve your own pull is not allowed"),
|
|
{"id": 202},
|
|
]
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
[
|
|
"42",
|
|
"oblachno-oss/devx",
|
|
"--event",
|
|
"APPROVE",
|
|
"--body",
|
|
"x" * 60,
|
|
"--checklist-confirmed",
|
|
"--checklist-categories",
|
|
"1,2,3,4,5,6,7,8",
|
|
],
|
|
env={"REVIEWER_GITEA_API_TOKEN": "fake-reviewer"},
|
|
)
|
|
assert result.exit_code == 0
|
|
assert "Review #202" in result.output
|
|
# Without CI_GITEA_API_TOKEN, the fallback is COMMENT
|
|
assert "Self-approval not allowed. Posting COMMENT instead." in result.output
|
|
assert client.create_review.call_count == 2
|
|
assert client.create_review.call_args_list[1].kwargs.get("event") == "COMMENT"
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_manual_review_self_approval_falls_back_to_ci_token(self, mock_client_class: MagicMock) -> None:
|
|
"""Self-approval with CI token available → retry APPROVE with CI token (different user)."""
|
|
client = mock_client_class.return_value
|
|
client.create_review.side_effect = [
|
|
APIError(422, "approve your own pull is not allowed"),
|
|
{"id": 303},
|
|
]
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
[
|
|
"42",
|
|
"oblachno-oss/devx",
|
|
"--event",
|
|
"APPROVE",
|
|
"--body",
|
|
"x" * 60,
|
|
"--checklist-confirmed",
|
|
"--checklist-categories",
|
|
"1,2,3,4,5,6,7,8",
|
|
],
|
|
env={"REVIEWER_GITEA_API_TOKEN": "fake-reviewer", "CI_GITEA_API_TOKEN": "fake-ci"},
|
|
)
|
|
assert result.exit_code == 0
|
|
assert "Review #303" in result.output
|
|
assert "Retrying with CI token" in result.output
|
|
# Second call should still be APPROVE (CI token retry)
|
|
assert client.create_review.call_count == 2
|
|
assert client.create_review.call_args_list[1].kwargs.get("event") == "APPROVE"
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_manual_review_ci_token_also_fails_falls_back_to_comment(self, mock_client_class: MagicMock) -> None:
|
|
"""Self-approval + CI token retry also fails → fall back to COMMENT."""
|
|
client = mock_client_class.return_value
|
|
client.create_review.side_effect = [
|
|
APIError(422, "approve your own pull is not allowed"),
|
|
APIError(422, "approve your own pull is not allowed"),
|
|
{"id": 404},
|
|
]
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
[
|
|
"42",
|
|
"oblachno-oss/devx",
|
|
"--event",
|
|
"APPROVE",
|
|
"--body",
|
|
"x" * 60,
|
|
"--checklist-confirmed",
|
|
"--checklist-categories",
|
|
"1,2,3,4,5,6,7,8",
|
|
],
|
|
env={"REVIEWER_GITEA_API_TOKEN": "fake-reviewer", "CI_GITEA_API_TOKEN": "fake-ci"},
|
|
)
|
|
assert result.exit_code == 0
|
|
assert "Review #404" in result.output
|
|
assert "CI token also cannot approve" in result.output
|
|
# Third call should be COMMENT (final fallback)
|
|
assert client.create_review.call_count == 3
|
|
assert client.create_review.call_args_list[2].kwargs.get("event") == "COMMENT"
|
|
|
|
@patch("devx.ci.pr_review.GiteaClient")
|
|
def test_manual_review_other_error_re_raises(self, mock_client_class: MagicMock) -> None:
|
|
client = mock_client_class.return_value
|
|
client.create_review.side_effect = APIError(500, "Internal server error")
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
main,
|
|
["42", "oblachno-oss/devx", "--event", "COMMENT", "--body", "x" * 60],
|
|
env={"CI_GITEA_TOKEN": "fake"},
|
|
)
|
|
assert result.exit_code != 0
|
|
|
|
|
|
def test_main_module_block() -> None:
|
|
import devx.ci.pr_review as pr
|
|
|
|
with patch.object(pr, "main") as mock_main:
|
|
with patch.object(pr, "__name__", "__main__"):
|
|
pr.main([])
|
|
mock_main.assert_called_once_with([])
|