diff --git a/commit_check/config.py b/commit_check/config.py index b4f5d1c3..5d627b8a 100644 --- a/commit_check/config.py +++ b/commit_check/config.py @@ -85,7 +85,7 @@ def _load_from_url(url: str) -> dict[str, Any]: import io return toml_load(io.BytesIO(data)) - except (urllib.error.URLError, urllib.error.HTTPError, Exception): + except urllib.error.URLError: return {} @@ -137,7 +137,7 @@ def load_config(path_hint: str = "") -> dict[str, Any]: URL before applying local overrides. """ if path_hint: - p = Path(path_hint) + p = Path(path_hint).resolve() if not p.exists(): raise FileNotFoundError(f"Specified config file not found: {path_hint}") with open(p, "rb") as f: diff --git a/commit_check/engine.py b/commit_check/engine.py index 58e3fd50..dcb08fae 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -98,42 +98,52 @@ def _should_skip_validation(self, context: ValidationContext) -> bool: and not has_commits() ) - def _should_skip_commit_validation(self, context: ValidationContext) -> bool: - """ - Determine if commit validation should be skipped. - - Skip if the current author or any co-author is in the ignore_authors list - for commits, or if no stdin_text, no commit_file, and no commits exist. - """ + def _author_in_ignore_list(self, context: ValidationContext) -> bool: + """Check if the current author or any co-author is in the ignore list.""" import re ignore_authors = context.config.get("commit", {}).get("ignore_authors", []) + if not ignore_authors: + return False + current_author = get_commit_info("an") if current_author and current_author in ignore_authors: return True # Check co-authors from the commit message body - if ignore_authors: - message = "" - if context.stdin_text: - message = context.stdin_text - elif context.commit_file: - try: - with open(context.commit_file, "r") as f: - message = f.read() - except (OSError, IOError): - pass - else: - message = get_commit_info("b") - if message: - co_authors = re.findall( - r"^Co-authored-by:\s*([^<\n]+?)\s*(?:<|$)", - message, - re.MULTILINE, - ) - for co_author in co_authors: - if co_author.strip() in ignore_authors: - return True + message = self._get_commit_body(context) + if not message: + return False + + co_authors = re.findall( + r"^Co-authored-by:\s*([^<\n]+)\s*(?:<|$)", + message, + re.MULTILINE, + ) + return any(co_author.strip() in ignore_authors for co_author in co_authors) + + @staticmethod + def _get_commit_body(context: ValidationContext) -> str: + """Retrieve the commit message body from context or git.""" + if context.stdin_text: + return context.stdin_text + if context.commit_file: + try: + with open(context.commit_file, "r") as f: + return f.read() + except (OSError, IOError): + pass + return get_commit_info("b") + + def _should_skip_commit_validation(self, context: ValidationContext) -> bool: + """ + Determine if commit validation should be skipped. + + Skip if the current author or any co-author is in the ignore_authors list + for commits, or if no stdin_text, no commit_file, and no commits exist. + """ + if self._author_in_ignore_list(context): + return True return ( context.stdin_text is None @@ -242,7 +252,7 @@ def _get_subject(self, context: ValidationContext) -> str: return get_commit_info("s") - def _validate_subject(self, subject: str) -> ValidationResult: + def _validate_subject(self, _subject: str) -> ValidationResult: """Override in subclasses for specific validation logic.""" return ValidationResult.PASS @@ -307,11 +317,11 @@ def _validate_subject(self, subject: str) -> ValidationResult: length = len(subject) constraint_value = self.rule.value - if self.rule.check == "subject_max_length" and length <= constraint_value: - return ValidationResult.PASS - elif self.rule.check == "subject_min_length" and length >= constraint_value: - return ValidationResult.PASS - elif self.rule.check not in ["subject_max_length", "subject_min_length"]: + if ( + (self.rule.check == "subject_max_length" and length <= constraint_value) + or (self.rule.check == "subject_min_length" and length >= constraint_value) + or self.rule.check not in ["subject_max_length", "subject_min_length"] + ): return ValidationResult.PASS self._print_failure(subject, f"length={length}, constraint={constraint_value}") diff --git a/commit_check/main.py b/commit_check/main.py index f88453ba..3fe46293 100644 --- a/commit_check/main.py +++ b/commit_check/main.py @@ -376,6 +376,89 @@ def _get_message_content( return None +def _resolve_commit_message_source( + args: argparse.Namespace, + stdin_reader: StdinReader, +) -> tuple[str | None, str | None]: + """Determine commit message source: file path or stdin content. + + Returns a tuple of (stdin_content, commit_file_path). + """ + if not args.message: + return None, None + + if args.commit_msg_file: + return None, args.commit_msg_file + + stdin_content = stdin_reader.read_piped_input() + return stdin_content or None, None + + +def _resolve_stdin_for_non_message( + args: argparse.Namespace, stdin_reader: StdinReader +) -> str | None: + """Resolve stdin content for non-message validation types.""" + has_non_message_check = any( + [args.branch, args.author_name, args.author_email, args.no_force_push] + ) + if not has_non_message_check: + return None + + stdin_content = stdin_reader.read_piped_input() + if args.no_force_push and stdin_content is None: + return _build_pre_commit_push_input() + return stdin_content + + +def _get_requested_checks(args: argparse.Namespace) -> list[str]: + """Build the list of requested validation checks based on CLI args.""" + requested_checks: list[str] = [] + + if args.message: + requested_checks.extend( + [ + "message", + "subject_imperative", + "subject_max_length", + "subject_min_length", + "require_signed_off_by", + "subject_capitalized", + "require_body", + "allow_merge_commits", + "allow_revert_commits", + "allow_empty_commits", + "allow_fixup_commits", + "allow_wip_commits", + ] + ) + if args.branch: + requested_checks.extend(["branch", "merge_base"]) + if args.author_name: + requested_checks.append("author_name") + if args.author_email: + requested_checks.append("author_email") + if args.no_force_push: + requested_checks.append("no_force_push") + + return requested_checks + + +def _run_json_output(engine: ValidationEngine, context: ValidationContext) -> int: + """Run validation and print JSON output.""" + outcomes: list[CheckOutcome] = engine.validate_all_detailed(context) + overall = "fail" if any(o.status == "fail" for o in outcomes) else "pass" + print( + json.dumps( + { + "status": overall, + "checks": [o.to_dict() for o in outcomes], + }, + indent=2, + ) + ) + return 0 if overall == "pass" else 1 + + def main() -> int: """The main entrypoint of commit-check program.""" parser = _get_parser() @@ -387,6 +470,10 @@ def main() -> int: stdin_reader = StdinReader() try: + # Handle positional commit_msg_file argument for pre-commit compatibility + if args.commit_msg_file: + args.message = True + # Load and merge configuration from all sources: CLI > Env > TOML > Defaults config_data = ConfigMerger.from_all_sources(args, args.config) @@ -399,81 +486,24 @@ def main() -> int: rule_builder = RuleBuilder(config_data) all_rules = rule_builder.build_all_rules() - # Handle positional commit_msg_file argument for pre-commit compatibility - # Store the file path separately from the boolean flag - commit_msg_file_path = None - if args.commit_msg_file: - commit_msg_file_path = args.commit_msg_file - # If a file was provided positionally, always enable message checking - args.message = True - - # Filter rules based on CLI arguments - requested_checks = [] - if args.message: # args.message is now a boolean flag - # Add commit message related checks - requested_checks.extend( - [ - "message", - "subject_imperative", - "subject_max_length", - "subject_min_length", - "require_signed_off_by", - "subject_capitalized", - "require_body", - "allow_merge_commits", - "allow_revert_commits", - "allow_empty_commits", - "allow_fixup_commits", - "allow_wip_commits", - ] - ) - if args.branch: - requested_checks.extend(["branch", "merge_base"]) - if args.author_name: - requested_checks.append("author_name") - if args.author_email: - requested_checks.append("author_email") - if args.no_force_push: - requested_checks.append("no_force_push") - - # If no specific checks requested, show help + # Determine which checks to run + requested_checks = _get_requested_checks(args) if not requested_checks: parser.print_help() return 0 # Filter rules to only include requested checks filtered_rules = [rule for rule in all_rules if rule.check in requested_checks] - - # Create validation engine with filtered rules engine = ValidationEngine(filtered_rules) - # Create validation context - stdin_content = None - commit_file_path = None - - if args.message: # args.message is a boolean flag - # Check if we have a file path from positional argument - if commit_msg_file_path: - commit_file_path = commit_msg_file_path - else: - # No file path provided, try reading from stdin - stdin_content = stdin_reader.read_piped_input() - if not stdin_content: - # No stdin and no file - let validators get data from git themselves - stdin_content = None - elif not any( - [args.branch, args.author_name, args.author_email, args.no_force_push] - ): - # If no specific validation type is requested, don't read stdin - pass - else: - # For non-message validations (branch, author, push), check for stdin input - stdin_content = stdin_reader.read_piped_input() - if args.no_force_push and stdin_content is None: - stdin_content = _build_pre_commit_push_input() - - # Reset banner state for this run so that multiple main() calls - # in the same process (e.g. tests) don't share banner state. + # Resolve validation context inputs + stdin_content, commit_file_path = _resolve_commit_message_source( + args, stdin_reader + ) + if not args.message: + stdin_content = _resolve_stdin_for_non_message(args, stdin_reader) + + # Reset banner state for this run from commit_check.util import print_error_header as _peh _peh.has_been_called = False @@ -490,22 +520,9 @@ def main() -> int: # Run validation – choose output mode based on --format output_format: str = getattr(args, "output_format", "text") if output_format == "json": - outcomes: list[CheckOutcome] = engine.validate_all_detailed(context) - overall = "fail" if any(o.status == "fail" for o in outcomes) else "pass" - print( - json.dumps( - { - "status": overall, - "checks": [o.to_dict() for o in outcomes], - }, - indent=2, - ) - ) - return 0 if overall == "pass" else 1 + return _run_json_output(engine, context) result = engine.validate_all(context) - - # Return appropriate exit code return 0 if result == ValidationResult.PASS else 1 except FileNotFoundError as e: diff --git a/commit_check/rule_builder.py b/commit_check/rule_builder.py index 2ec246a9..506502ac 100644 --- a/commit_check/rule_builder.py +++ b/commit_check/rule_builder.py @@ -259,13 +259,13 @@ def _build_boolean_rule( # For "allow_*" rules, only create rule if they're disabled (False) # For "require_*" rules, only create rule if they're enabled (True) - if check.startswith("allow_") and config_value is True: - return None - elif check.startswith("require_") and config_value is False: - return None - elif ( - check in ["subject_capitalized", "subject_imperative"] - and config_value is False + if ( + (check.startswith("allow_") and config_value is True) + or (check.startswith("require_") and config_value is False) + or ( + check in ["subject_capitalized", "subject_imperative"] + and config_value is False + ) ): return None @@ -295,7 +295,7 @@ def _get_allowed_branch_names(self) -> list[str]: def _build_conventional_commit_regex(self, allowed_types: list[str]) -> str: """Build regex for conventional commit messages.""" types_pattern = "|".join(sorted(set(allowed_types))) - return rf"^({types_pattern}){{1}}(\([\w\-\.]+\))?(!)?: ([\w ])+([\s\S]*)|(Merge).*|(fixup!.*)" + return rf"^({types_pattern})(\([\w\-\.]+\))?(!)?: ([\w ])+([\s\S]*)|(Merge).*|(fixup!.*)" def _build_conventional_branch_regex( self, allowed_types: list[str], allowed_names: list[str] diff --git a/docs/conf.py b/docs/conf.py index bc75624c..62190a67 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -84,7 +84,7 @@ } object_description_options = [ - ("py:parameter", dict(include_in_toc=False)), + ("py:parameter", {"include_in_toc": False}), ] sphinx_immaterial_custom_admonitions = [ @@ -102,7 +102,7 @@ ] for name in ("hint", "tip", "important"): sphinx_immaterial_custom_admonitions.append( - dict(name=name, icon="material/school", override=True) + {"name": name, "icon": "material/school", "override": True} ) diff --git a/tests/config_test.py b/tests/config_test.py index 28224ae4..3deb8af9 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -15,6 +15,10 @@ _github_shorthand_to_url, ) +# String constants used across tests +URLOPEN_MODULE = "urllib.request.urlopen" +EXAMPLE_CONFIG_URL = "https://example.com/cchk.toml" + class TestConfig: @pytest.mark.benchmark @@ -596,9 +600,9 @@ def test_inherit_from_url_success(self): mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) - with patch("urllib.request.urlopen", return_value=mock_response): + with patch(URLOPEN_MODULE, return_value=mock_response): config = { - "inherit_from": "https://example.com/cchk.toml", + "inherit_from": EXAMPLE_CONFIG_URL, "commit": {"subject_max_length": 72}, } result = _resolve_inherit_from(config) @@ -609,11 +613,11 @@ def test_inherit_from_url_failure_is_ignored(self): import urllib.error with patch( - "urllib.request.urlopen", + URLOPEN_MODULE, side_effect=urllib.error.URLError("network error"), ): config = { - "inherit_from": "https://example.com/cchk.toml", + "inherit_from": EXAMPLE_CONFIG_URL, "fallback": True, } result = _resolve_inherit_from(config) @@ -627,7 +631,7 @@ def test_inherit_from_http_url_is_rejected(self): "fallback": True, } # urlopen should NOT be called for http:// URLs - with patch("urllib.request.urlopen") as mock_urlopen: + with patch(URLOPEN_MODULE) as mock_urlopen: result = _resolve_inherit_from(config) mock_urlopen.assert_not_called() assert result == {"fallback": True} @@ -641,7 +645,7 @@ def test_inherit_from_github_shorthand(self): mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) - with patch("urllib.request.urlopen", return_value=mock_response) as mock_open: + with patch(URLOPEN_MODULE, return_value=mock_response) as mock_open: config = { "inherit_from": "github:my-org/.github:cchk.toml", "commit": {"subject_max_length": 72}, @@ -665,7 +669,7 @@ def test_inherit_from_github_shorthand_with_ref(self): mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) - with patch("urllib.request.urlopen", return_value=mock_response) as mock_open: + with patch(URLOPEN_MODULE, return_value=mock_response) as mock_open: config = { "inherit_from": "github:my-org/.github@main:cchk.toml", } @@ -727,8 +731,8 @@ def test_load_from_url_success(self): mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) - with patch("urllib.request.urlopen", return_value=mock_response): - result = _load_from_url("https://example.com/cchk.toml") + with patch(URLOPEN_MODULE, return_value=mock_response): + result = _load_from_url(EXAMPLE_CONFIG_URL) assert result == {"commit": {"conventional_commits": True}} @pytest.mark.benchmark @@ -736,10 +740,10 @@ def test_load_from_url_network_error(self): import urllib.error with patch( - "urllib.request.urlopen", + URLOPEN_MODULE, side_effect=urllib.error.URLError("network error"), ): - result = _load_from_url("https://example.com/cchk.toml") + result = _load_from_url(EXAMPLE_CONFIG_URL) assert result == {} @pytest.mark.benchmark @@ -747,12 +751,12 @@ def test_load_from_url_http_error(self): import urllib.error with patch( - "urllib.request.urlopen", + URLOPEN_MODULE, side_effect=urllib.error.HTTPError( - "https://example.com/cchk.toml", 404, "Not Found", {}, None + EXAMPLE_CONFIG_URL, 404, "Not Found", {}, None ), ): - result = _load_from_url("https://example.com/cchk.toml") + result = _load_from_url(EXAMPLE_CONFIG_URL) assert result == {} diff --git a/tests/engine_test.py b/tests/engine_test.py index 3143c9f4..024b2b28 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -23,6 +23,15 @@ ) from commit_check.rule_builder import ValidationRule +# String constants used across tests (defined once to avoid duplication) +GIT_CONFIG_VALUE = "commit_check.engine.get_git_config_value" +FETCH_REMOTE_REF = "commit_check.engine.fetch_remote_ref" +GET_GIT_REMOTES = "commit_check.engine.get_git_remotes" +REFS_HEADS_MAIN = "refs/heads/main" +CONVENTIONAL_COMMIT_REGEX = r"^(feat|fix): .+" +BAD_COMMIT_MSG = "Bad commit" +USE_CONVENTIONAL_FORMAT = "Use conventional format" + class TestValidationResult: @pytest.mark.benchmark @@ -136,8 +145,9 @@ def test_commit_message_validator_from_git(self, mock_get_commit_info): result = validator.validate(context) assert result == ValidationResult.PASS - # Should call get_commit_info three times: subject, body, and author - assert mock_get_commit_info.call_count == 3 + # Should call get_commit_info twice: subject and body + # (author lookup is skipped when ignore_authors list is empty) + assert mock_get_commit_info.call_count == 2 @patch("commit_check.engine.has_commits") @patch("commit_check.engine.get_commit_info") @@ -330,7 +340,7 @@ def test_validate_without_regex(self): class TestAuthorValidator: @patch("commit_check.engine.has_commits") - @patch("commit_check.engine.get_git_config_value") + @patch(GIT_CONFIG_VALUE) @patch("commit_check.engine.get_commit_info") @pytest.mark.benchmark def test_author_validator_name_valid( @@ -348,7 +358,7 @@ def test_author_validator_name_valid( assert result == ValidationResult.PASS @patch("commit_check.engine.has_commits") - @patch("commit_check.engine.get_git_config_value") + @patch(GIT_CONFIG_VALUE) @patch("commit_check.engine.get_commit_info") @pytest.mark.benchmark def test_author_validator_email_valid( @@ -434,7 +444,7 @@ def test_get_author_value_with_email_format(self): context = ValidationContext() with ( - patch("commit_check.engine.get_git_config_value", return_value=""), + patch(GIT_CONFIG_VALUE, return_value=""), patch( "commit_check.engine.get_commit_info", return_value="test@example.com" ), @@ -1049,9 +1059,9 @@ def test_co_author_in_ignore_list_skips_validation(self): """Test that a commit with a co-author in ignore_authors is skipped.""" rule = ValidationRule( check="message", - regex=r"^(feat|fix): .+", - error="Bad commit", - suggest="Use conventional format", + regex=CONVENTIONAL_COMMIT_REGEX, + error=BAD_COMMIT_MSG, + suggest=USE_CONVENTIONAL_FORMAT, ) validator = CommitMessageValidator(rule) @@ -1068,9 +1078,9 @@ def test_co_author_not_in_ignore_list_does_not_skip(self): """Test that co-author not in ignore list does not bypass validation.""" rule = ValidationRule( check="message", - regex=r"^(feat|fix): .+", - error="Bad commit", - suggest="Use conventional format", + regex=CONVENTIONAL_COMMIT_REGEX, + error=BAD_COMMIT_MSG, + suggest=USE_CONVENTIONAL_FORMAT, ) validator = CommitMessageValidator(rule) @@ -1091,9 +1101,9 @@ def test_co_author_in_ignore_list_from_commit_file(self): rule = ValidationRule( check="message", - regex=r"^(feat|fix): .+", - error="Bad commit", - suggest="Use conventional format", + regex=CONVENTIONAL_COMMIT_REGEX, + error=BAD_COMMIT_MSG, + suggest=USE_CONVENTIONAL_FORMAT, ) validator = CommitMessageValidator(rule) @@ -1133,7 +1143,7 @@ def test_author_name_uses_git_config_when_available(self): with ( patch("commit_check.engine.get_commit_info", return_value="some-author"), patch( - "commit_check.engine.get_git_config_value", + GIT_CONFIG_VALUE, return_value="01 Invalid Name", ), ): @@ -1154,7 +1164,7 @@ def test_author_name_falls_back_to_git_log_when_config_empty(self): context = ValidationContext() with ( - patch("commit_check.engine.get_git_config_value", return_value=""), + patch(GIT_CONFIG_VALUE, return_value=""), patch("commit_check.engine.get_commit_info", return_value="Valid Name"), ): result = validator.validate(context) @@ -1175,7 +1185,7 @@ def test_author_email_uses_git_config_when_available(self): with ( patch("commit_check.engine.get_commit_info", return_value="some-author"), patch( - "commit_check.engine.get_git_config_value", + GIT_CONFIG_VALUE, return_value="user@example.com", ), ): @@ -1356,18 +1366,14 @@ def test_git_error_allows_push(self): context = ValidationContext(stdin_text=push_info) with patch("commit_check.engine.git_merge_base", return_value=128): - with patch( - "commit_check.engine.fetch_remote_ref", return_value=False - ) as mock_fetch: - with patch( - "commit_check.engine.get_git_remotes", return_value=["origin"] - ): + with patch(FETCH_REMOTE_REF, return_value=False) as mock_fetch: + with patch(GET_GIT_REMOTES, return_value=["origin"]): with patch( "commit_check.engine.get_upstream_branch", return_value="" ): result = validator.validate(context) - mock_fetch.assert_called_once_with("origin", "refs/heads/main") + mock_fetch.assert_called_once_with("origin", REFS_HEADS_MAIN) assert result == ValidationResult.PASS @pytest.mark.benchmark @@ -1382,17 +1388,13 @@ def test_missing_remote_sha_is_fetched_then_force_push_is_blocked(self): "commit_check.engine.git_merge_base", side_effect=[128, 1] ) as mock_merge: with patch("commit_check.engine.get_upstream_branch", return_value=""): - with patch( - "commit_check.engine.get_git_remotes", return_value=["origin"] - ): - with patch( - "commit_check.engine.fetch_remote_ref", return_value=True - ) as mock_fetch: + with patch(GET_GIT_REMOTES, return_value=["origin"]): + with patch(FETCH_REMOTE_REF, return_value=True) as mock_fetch: with patch("commit_check.util._print_failure"): result = validator.validate(context) assert mock_merge.call_count == 2 - mock_fetch.assert_called_once_with("origin", "refs/heads/main") + mock_fetch.assert_called_once_with("origin", REFS_HEADS_MAIN) assert result == ValidationResult.FAIL @pytest.mark.benchmark @@ -1408,15 +1410,13 @@ def test_missing_remote_sha_fetch_prefers_matching_upstream_remote(self): "commit_check.engine.get_upstream_branch", return_value="upstream/main" ): with patch( - "commit_check.engine.get_git_remotes", + GET_GIT_REMOTES, return_value=["origin", "upstream"], ): - with patch( - "commit_check.engine.fetch_remote_ref", return_value=True - ) as mock_fetch: + with patch(FETCH_REMOTE_REF, return_value=True) as mock_fetch: result = validator.validate(context) - mock_fetch.assert_called_once_with("upstream", "refs/heads/main") + mock_fetch.assert_called_once_with("upstream", REFS_HEADS_MAIN) assert result == ValidationResult.PASS @pytest.mark.benchmark @@ -1432,19 +1432,17 @@ def test_missing_remote_sha_tries_next_remote_until_resolved(self): ) as mock_merge: with patch("commit_check.engine.get_upstream_branch", return_value=""): with patch( - "commit_check.engine.get_git_remotes", + GET_GIT_REMOTES, return_value=["origin", "upstream"], ): - with patch( - "commit_check.engine.fetch_remote_ref", return_value=True - ) as mock_fetch: + with patch(FETCH_REMOTE_REF, return_value=True) as mock_fetch: with patch("commit_check.util._print_failure"): result = validator.validate(context) assert mock_merge.call_count == 3 assert [call.args for call in mock_fetch.call_args_list] == [ - ("origin", "refs/heads/main"), - ("upstream", "refs/heads/main"), + ("origin", REFS_HEADS_MAIN), + ("upstream", REFS_HEADS_MAIN), ] assert result == ValidationResult.FAIL diff --git a/tests/main_test.py b/tests/main_test.py index 7c3872df..0302338f 100644 --- a/tests/main_test.py +++ b/tests/main_test.py @@ -12,6 +12,7 @@ ) CMD = "commit-check" +FEATURE_TOPIC_BRANCH = "feature/topic" class TestMain: @@ -849,7 +850,7 @@ def test_no_force_push_uses_pre_commit_env_before_upstream(self, mocker): mocker.patch.dict( os.environ, { - "PRE_COMMIT_LOCAL_BRANCH": "feature/topic", + "PRE_COMMIT_LOCAL_BRANCH": FEATURE_TOPIC_BRANCH, "PRE_COMMIT_REMOTE_BRANCH": "main", "PRE_COMMIT_TO_REF": "local-sha", "PRE_COMMIT_FROM_REF": "remote-sha", @@ -873,7 +874,7 @@ def test_no_force_push_pre_commit_env_fetches_remote_sha(self, mocker): os.environ, { "PRE_COMMIT_REMOTE_NAME": "upstream", - "PRE_COMMIT_LOCAL_BRANCH": "feature/topic", + "PRE_COMMIT_LOCAL_BRANCH": FEATURE_TOPIC_BRANCH, "PRE_COMMIT_REMOTE_BRANCH": "main", "PRE_COMMIT_TO_REF": "local-sha", }, @@ -907,7 +908,7 @@ def test_build_pre_commit_push_input_normalizes_branch_names(self, mocker): mocker.patch.dict( os.environ, { - "PRE_COMMIT_LOCAL_BRANCH": "feature/topic", + "PRE_COMMIT_LOCAL_BRANCH": FEATURE_TOPIC_BRANCH, "PRE_COMMIT_REMOTE_BRANCH": "main", "PRE_COMMIT_TO_REF": "local-sha", "PRE_COMMIT_FROM_REF": "remote-sha", @@ -927,7 +928,7 @@ def test_build_pre_commit_push_input_prefers_remote_sha(self, mocker): os.environ, { "PRE_COMMIT_REMOTE_NAME": "upstream", - "PRE_COMMIT_LOCAL_BRANCH": "feature/topic", + "PRE_COMMIT_LOCAL_BRANCH": FEATURE_TOPIC_BRANCH, "PRE_COMMIT_REMOTE_BRANCH": "main", "PRE_COMMIT_TO_REF": "local-sha", "PRE_COMMIT_FROM_REF": "range-base-sha", diff --git a/tests/rule_builder_test.py b/tests/rule_builder_test.py index 2663b951..27d37128 100644 --- a/tests/rule_builder_test.py +++ b/tests/rule_builder_test.py @@ -4,6 +4,9 @@ from commit_check.rules_catalog import RuleCatalogEntry import pytest +# String constants used across tests +BAD_FORMAT_ERROR = "Bad format" + class TestValidationRule: @pytest.mark.benchmark @@ -239,13 +242,13 @@ def test_message_pattern_takes_precedence(self): builder = RuleBuilder(config) catalog_entry = RuleCatalogEntry( - check="message", regex="", error="Bad format", suggest="Use JIRA format" + check="message", regex="", error=BAD_FORMAT_ERROR, suggest="Use JIRA format" ) rule = builder._build_conventional_commit_rule(catalog_entry) assert rule is not None assert rule.regex == r"^PROJ-\d+: .+" - assert rule.error == "Bad format" + assert rule.error == BAD_FORMAT_ERROR assert "required pattern" in rule.suggest @pytest.mark.benchmark @@ -260,7 +263,10 @@ def test_message_pattern_overrides_conventional_commits(self): builder = RuleBuilder(config) catalog_entry = RuleCatalogEntry( - check="message", regex="", error="Bad format", suggest="Use correct format" + check="message", + regex="", + error=BAD_FORMAT_ERROR, + suggest="Use correct format", ) rule = builder._build_conventional_commit_rule(catalog_entry) @@ -280,7 +286,7 @@ def test_message_pattern_empty_falls_back(self): builder = RuleBuilder(config) catalog_entry = RuleCatalogEntry( - check="message", regex="", error="Bad format", suggest="..." + check="message", regex="", error=BAD_FORMAT_ERROR, suggest="..." ) rule = builder._build_conventional_commit_rule(catalog_entry) diff --git a/tests/util_test.py b/tests/util_test.py index ecab6f73..8579fdf4 100644 --- a/tests/util_test.py +++ b/tests/util_test.py @@ -26,6 +26,10 @@ from subprocess import CalledProcessError, PIPE from unittest.mock import MagicMock, patch +# String constants used across tests +REFS_HEADS_MAIN = "refs/heads/main" +USER_NAME_CONFIG = "user.name" + class TestUtil: class TestGetBranchName: @@ -199,7 +203,7 @@ def test_get_upstream_remote_sha(self, mocker): result = get_upstream_remote_sha("origin/main") mock_run.assert_called_once_with( - ["git", "ls-remote", "--exit-code", "origin", "refs/heads/main"], + ["git", "ls-remote", "--exit-code", "origin", REFS_HEADS_MAIN], stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8", @@ -262,7 +266,7 @@ def test_get_remote_branch_sha(self, mocker): result = get_remote_branch_sha("origin", "main") mock_run.assert_called_once_with( - ["git", "ls-remote", "--exit-code", "origin", "refs/heads/main"], + ["git", "ls-remote", "--exit-code", "origin", REFS_HEADS_MAIN], stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8", @@ -364,9 +368,9 @@ def test_fetch_remote_ref(self, mocker): )(), ) - assert fetch_remote_ref("origin", "refs/heads/main") is True + assert fetch_remote_ref("origin", REFS_HEADS_MAIN) is True mock_run.assert_called_once_with( - ["git", "fetch", "--quiet", "--no-tags", "origin", "refs/heads/main"], + ["git", "fetch", "--quiet", "--no-tags", "origin", REFS_HEADS_MAIN], stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8", @@ -383,13 +387,13 @@ def test_fetch_remote_ref_failure(self, mocker): )(), ) - assert fetch_remote_ref("origin", "refs/heads/main") is False + assert fetch_remote_ref("origin", REFS_HEADS_MAIN) is False @pytest.mark.benchmark @pytest.mark.parametrize( "remote_name,remote_ref", [ - ("", "refs/heads/main"), + ("", REFS_HEADS_MAIN), ("origin", ""), ], ) @@ -859,7 +863,7 @@ def test_get_git_config_value_success(self, mocker): from commit_check.util import get_git_config_value mocker.patch("commit_check.util.cmd_output", return_value="John Doe\n") - result = get_git_config_value("user.name") + result = get_git_config_value(USER_NAME_CONFIG) assert result == "John Doe" @pytest.mark.benchmark @@ -868,7 +872,7 @@ def test_get_git_config_value_not_set(self, mocker): from commit_check.util import get_git_config_value mocker.patch("commit_check.util.cmd_output", return_value="") - result = get_git_config_value("user.name") + result = get_git_config_value(USER_NAME_CONFIG) assert result == "" @pytest.mark.benchmark @@ -882,7 +886,7 @@ def test_get_git_config_value_exception(self, mocker): returncode=1, cmd="git config --get user.name" ), ) - result = get_git_config_value("user.name") + result = get_git_config_value(USER_NAME_CONFIG) assert result == "" @pytest.mark.benchmark