From d648cdc078289f0139b6b5c345f5849b7555c969 Mon Sep 17 00:00:00 2001 From: Tony Narlock Date: Sun, 1 Mar 2026 16:07:47 -0600 Subject: [PATCH] refactor(test[import]) Extract _run_import_defaults to reduce boilerplate why: 63+ call sites repeat identical default kwargs, obscuring test intent. what: - Add _run_import_defaults() helper with test-friendly defaults - Replace all _run_import() calls with _run_import_defaults() - Parameterize 3 structurally identical --sync/--prune test pairs --- tests/cli/test_import_repos.py | 664 +++++++++++---------------------- 1 file changed, 228 insertions(+), 436 deletions(-) diff --git a/tests/cli/test_import_repos.py b/tests/cli/test_import_repos.py index 0df5ebdc..51fbcadb 100644 --- a/tests/cli/test_import_repos.py +++ b/tests/cli/test_import_repos.py @@ -55,6 +55,105 @@ def _make_repo( ) +def _run_import_defaults( + importer: t.Any, + *, + service_name: str = "github", + target: str = "testuser", + workspace: str = "", + mode: str = "user", + language: str | None = None, + topics: str | None = None, + min_stars: int = 0, + include_archived: bool = False, + include_forks: bool = False, + limit: int = 100, + config_path_str: str | None = None, + dry_run: bool = False, + yes: bool = True, + output_json: bool = False, + output_ndjson: bool = False, + color: str = "never", + **extra_kwargs: t.Any, +) -> int: + """Call _run_import with test-friendly defaults for 9 filter/display kwargs. + + The 9 kwargs with test-friendly defaults are: ``language``, ``topics``, + ``min_stars``, ``include_archived``, ``include_forks``, ``limit``, + ``output_json``, ``output_ndjson``, and ``color``. Callers only need + to supply kwargs that differ from these defaults. + + Parameters + ---------- + importer : t.Any + Mock importer instance (satisfies the Importer protocol). + service_name : str + Service name, defaults to ``"github"``. + target : str + Target user/org/query, defaults to ``"testuser"``. + workspace : str + Workspace root directory path. + mode : str + Import mode, defaults to ``"user"``. + language : str | None + Language filter, defaults to ``None``. + topics : str | None + Topics filter, defaults to ``None``. + min_stars : int + Minimum star count, defaults to ``0``. + include_archived : bool + Include archived repos, defaults to ``False``. + include_forks : bool + Include forked repos, defaults to ``False``. + limit : int + Repo fetch limit, defaults to ``100``. + config_path_str : str | None + Config file path, defaults to ``None``. + dry_run : bool + Dry-run mode, defaults to ``False``. + yes : bool + Auto-confirm, defaults to ``True``. + output_json : bool + JSON output, defaults to ``False``. + output_ndjson : bool + NDJSON output, defaults to ``False``. + color : str + Color mode, defaults to ``"never"``. + **extra_kwargs : t.Any + Forwarded to ``_run_import`` (e.g. ``use_https``, ``flatten_groups``). + + Returns + ------- + int + Exit code from ``_run_import``. + + Examples + -------- + >>> _run_import_defaults.__name__ + '_run_import_defaults' + """ + return _run_import( + importer, + service_name=service_name, + target=target, + workspace=workspace, + mode=mode, + language=language, + topics=topics, + min_stars=min_stars, + include_archived=include_archived, + include_forks=include_forks, + limit=limit, + config_path_str=config_path_str, + dry_run=dry_run, + yes=yes, + output_json=output_json, + output_ndjson=output_ndjson, + color=color, + **extra_kwargs, + ) + + class MockImporter: """Reusable mock importer for tests.""" @@ -368,24 +467,16 @@ def test_import_repos( importer = MockImporter(repos=mock_repos, error=mock_error) - _run_import( + _run_import_defaults( importer, service_name=service_name, target=target, workspace=str(workspace), mode=mode, - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), dry_run=dry_run, yes=yes, output_json=output_json, - output_ndjson=False, - color="never", ) for expected_text in expected_log_contains: @@ -426,24 +517,11 @@ def test_import_repos_user_abort( importer = MockImporter(repos=[_make_repo("repo1")]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, yes=False, # Require confirmation - output_json=False, - output_ndjson=False, - color="never", ) assert "Aborted by user" in caplog.text @@ -476,24 +554,11 @@ def raise_eof(_: str) -> str: importer = MockImporter(repos=[_make_repo("repo1")]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, yes=False, - output_json=False, - output_ndjson=False, - color="never", ) assert "Aborted by user" in caplog.text @@ -520,24 +585,11 @@ def test_import_repos_non_tty_aborts( importer = MockImporter(repos=[_make_repo("repo1")]) - result = _run_import( + result = _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, yes=False, - output_json=False, - output_ndjson=False, - color="never", ) assert result == 1, "Non-interactive abort must return non-zero exit code" @@ -570,24 +622,10 @@ def test_import_repos_skips_existing( importer = MockImporter(repos=[_make_repo("repo1"), _make_repo("repo2")]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert "Added 1 repositories" in caplog.text @@ -623,24 +661,10 @@ def test_import_repos_all_existing( importer = MockImporter(repos=[_make_repo("repo1")]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert "All repositories already exist" in caplog.text @@ -658,24 +682,12 @@ def test_import_repos_json_output( importer = MockImporter(repos=[_make_repo("repo1", stars=50)]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.yaml"), dry_run=True, - yes=True, output_json=True, - output_ndjson=False, - color="never", ) captured = capsys.readouterr() @@ -699,24 +711,12 @@ def test_import_repos_ndjson_output( importer = MockImporter(repos=[_make_repo("repo1"), _make_repo("repo2")]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.yaml"), dry_run=True, - yes=True, - output_json=False, output_ndjson=True, - color="never", ) captured = capsys.readouterr() @@ -751,12 +751,9 @@ def fetch_repos( received_options.append(options) return iter([]) - _run_import( + _run_import_defaults( CapturingImporter(), - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", language="Python", topics="cli,tool,python", min_stars=50, @@ -765,10 +762,6 @@ def fetch_repos( limit=200, config_path_str=str(tmp_path / "config.yaml"), dry_run=True, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert len(received_options) == 1 @@ -798,24 +791,13 @@ def test_import_repos_codecommit_no_target_required( repos=[_make_repo("aws-repo")], ) - _run_import( + _run_import_defaults( importer, service_name="codecommit", target="", # Empty target is OK for CodeCommit workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.yaml"), dry_run=True, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) # Should succeed and find repos @@ -841,24 +823,11 @@ def test_import_repos_many_repos_truncates_preview( importer = MockImporter(repos=many_repos) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.yaml"), dry_run=True, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert "Found 15 repositories" in caplog.text @@ -883,24 +852,10 @@ def test_import_repos_config_load_error( importer = MockImporter(repos=[_make_repo("repo1")]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert "Error loading config" in caplog.text @@ -917,59 +872,45 @@ def test_import_no_args_shows_help(capsys: pytest.CaptureFixture[str]) -> None: assert "Import repositories from remote services" in captured.out -def test_import_repos_defaults_to_ssh_urls( - tmp_path: pathlib.Path, - monkeypatch: MonkeyPatch, - caplog: pytest.LogCaptureFixture, -) -> None: - """Test _run_import writes SSH URLs to config by default.""" - import yaml - - caplog.set_level(logging.INFO) - - monkeypatch.setenv("HOME", str(tmp_path)) - workspace = tmp_path / "repos" - workspace.mkdir() - config_file = tmp_path / ".vcspull.yaml" - - importer = MockImporter(repos=[_make_repo("myrepo")]) +class UrlSchemeFixture(t.NamedTuple): + """Fixture for SSH vs HTTPS URL scheme test cases.""" - _run_import( - importer, - service_name="github", - target="testuser", - workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, - config_path_str=str(config_file), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", - ) + test_id: str + use_https: bool + expected_url: str - assert config_file.exists() - with config_file.open() as f: - config = yaml.safe_load(f) - repo_url = config["~/repos/"]["myrepo"]["repo"] - assert repo_url == "git+git@github.com:testuser/myrepo.git" +URL_SCHEME_FIXTURES: list[UrlSchemeFixture] = [ + UrlSchemeFixture( + test_id="defaults-to-ssh-urls", + use_https=False, + expected_url="git+git@github.com:testuser/myrepo.git", + ), + UrlSchemeFixture( + test_id="https-flag-writes-https-urls", + use_https=True, + expected_url="git+https://github.com/testuser/myrepo.git", + ), +] -def test_import_repos_https_flag( +@pytest.mark.parametrize( + list(UrlSchemeFixture._fields), + URL_SCHEME_FIXTURES, + ids=[f.test_id for f in URL_SCHEME_FIXTURES], +) +def test_import_repos_url_scheme( + test_id: str, + use_https: bool, + expected_url: str, tmp_path: pathlib.Path, monkeypatch: MonkeyPatch, caplog: pytest.LogCaptureFixture, ) -> None: - """Test _run_import writes HTTPS URLs when use_https=True.""" + """Test _run_import writes SSH or HTTPS URLs based on use_https flag.""" import yaml + del test_id caplog.set_level(logging.INFO) monkeypatch.setenv("HOME", str(tmp_path)) @@ -979,25 +920,11 @@ def test_import_repos_https_flag( importer = MockImporter(repos=[_make_repo("myrepo")]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", - use_https=True, + use_https=use_https, ) assert config_file.exists() @@ -1005,27 +932,50 @@ def test_import_repos_https_flag( config = yaml.safe_load(f) repo_url = config["~/repos/"]["myrepo"]["repo"] - assert repo_url == "git+https://github.com/testuser/myrepo.git" + assert repo_url == expected_url -def test_import_https_flag_via_cli() -> None: - """Test that --https flag is recognized by the CLI parser.""" - from vcspull.cli import create_parser +class UrlSchemeCliFixture(t.NamedTuple): + """Fixture for SSH vs HTTPS CLI flag test cases.""" - parser = create_parser(return_subparsers=False) - args = parser.parse_args( - ["import", "github", "testuser", "-w", "/tmp/repos", "--https"] - ) - assert args.use_https is True + test_id: str + extra_args: list[str] + expected_use_https: bool + + +URL_SCHEME_CLI_FIXTURES: list[UrlSchemeCliFixture] = [ + UrlSchemeCliFixture( + test_id="https-flag-via-cli", + extra_args=["--https"], + expected_use_https=True, + ), + UrlSchemeCliFixture( + test_id="ssh-default-via-cli", + extra_args=[], + expected_use_https=False, + ), +] -def test_import_ssh_default_via_cli() -> None: - """Test that SSH is the default (no --https flag).""" +@pytest.mark.parametrize( + list(UrlSchemeCliFixture._fields), + URL_SCHEME_CLI_FIXTURES, + ids=[f.test_id for f in URL_SCHEME_CLI_FIXTURES], +) +def test_import_url_scheme_via_cli( + test_id: str, + extra_args: list[str], + expected_use_https: bool, +) -> None: + """Test that --https flag controls use_https in the CLI parser.""" + del test_id from vcspull.cli import create_parser parser = create_parser(return_subparsers=False) - args = parser.parse_args(["import", "github", "testuser", "-w", "/tmp/repos"]) - assert args.use_https is False + args = parser.parse_args( + ["import", "github", "testuser", "-w", "/tmp/repos", *extra_args] + ) + assert args.use_https is expected_use_https def test_import_flatten_groups_flag_via_cli() -> None: @@ -1085,24 +1035,10 @@ def test_import_repos_rejects_unsupported_config_type( importer = MockImporter(repos=[_make_repo("repo1")]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.toml"), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert "Unsupported config file type" in caplog.text @@ -1134,24 +1070,10 @@ def raise_multiple_config(_: str | None) -> pathlib.Path: raise_multiple_config, ) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=None, - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert "Multiple configs" in caplog.text @@ -1171,101 +1093,75 @@ def test_import_repos_invalid_limit( importer = MockImporter(repos=[_make_repo("repo1")]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, limit=-1, config_path_str=str(tmp_path / "config.yaml"), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert "limit must be >= 0" in caplog.text -def test_import_repos_returns_nonzero_on_error( - tmp_path: pathlib.Path, - monkeypatch: MonkeyPatch, - caplog: pytest.LogCaptureFixture, -) -> None: - """Test _run_import returns non-zero exit code on error.""" - caplog.set_level(logging.ERROR) +class ExitCodeFixture(t.NamedTuple): + """Fixture for exit-code test cases (error vs success).""" - monkeypatch.setenv("HOME", str(tmp_path)) - workspace = tmp_path / "repos" - workspace.mkdir() - - importer = MockImporter(error=AuthenticationError("Bad credentials")) + test_id: str + mock_repos: list[RemoteRepo] + mock_error: Exception | None + expected_zero: bool - result = _run_import( - importer, - service_name="github", - target="testuser", - workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, - config_path_str=str(tmp_path / "config.yaml"), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", - ) - assert result != 0 +EXIT_CODE_FIXTURES: list[ExitCodeFixture] = [ + ExitCodeFixture( + test_id="returns-nonzero-on-error", + mock_repos=[], + mock_error=AuthenticationError("Bad credentials"), + expected_zero=False, + ), + ExitCodeFixture( + test_id="returns-zero-on-success", + mock_repos=[_make_repo("repo1")], + mock_error=None, + expected_zero=True, + ), +] -def test_import_repos_returns_zero_on_success( +@pytest.mark.parametrize( + list(ExitCodeFixture._fields), + EXIT_CODE_FIXTURES, + ids=[f.test_id for f in EXIT_CODE_FIXTURES], +) +def test_import_repos_exit_code( + test_id: str, + mock_repos: list[RemoteRepo], + mock_error: Exception | None, + expected_zero: bool, tmp_path: pathlib.Path, monkeypatch: MonkeyPatch, caplog: pytest.LogCaptureFixture, ) -> None: - """Test _run_import returns 0 on success.""" - caplog.set_level(logging.INFO) + """Test _run_import returns correct exit code on error vs success.""" + del test_id + caplog.set_level(logging.DEBUG) monkeypatch.setenv("HOME", str(tmp_path)) workspace = tmp_path / "repos" workspace.mkdir() - importer = MockImporter(repos=[_make_repo("repo1")]) + importer = MockImporter(repos=mock_repos, error=mock_error) - result = _run_import( + result = _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.yaml"), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) - assert result == 0 + if expected_zero: + assert result == 0 + else: + assert result != 0 def test_import_repos_json_config_write( @@ -1283,24 +1179,10 @@ def test_import_repos_json_config_write( importer = MockImporter(repos=[_make_repo("repo1")]) - result = _run_import( + result = _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert result == 0 @@ -1328,24 +1210,10 @@ def test_import_repos_rejects_non_dict_config( importer = MockImporter(repos=[_make_repo("repo1")]) - _run_import( + _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert "not a valid mapping" in caplog.text @@ -1369,24 +1237,10 @@ def test_import_repos_non_mapping_workspace_returns_error( importer = MockImporter(repos=[_make_repo("repo1")]) - result = _run_import( + result = _run_import_defaults( importer, - service_name="github", - target="testuser", workspace=str(workspace), - mode="user", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) assert result == 1 @@ -1547,24 +1401,13 @@ def test_import_nested_groups( importer = MockImporter(service_name="GitLab", repos=mock_repos) - _run_import( + _run_import_defaults( importer, service_name="gitlab", target=target, workspace=str(workspace), mode=mode, - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(config_file), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", flatten_groups=flatten_groups, ) @@ -1649,24 +1492,14 @@ def test_import_repos_language_warning( ) importer = MockImporter(service_name=display_name) - _run_import( + _run_import_defaults( importer, service_name=service_name, target="testuser" if service_name != "codecommit" else "", workspace=str(workspace), - mode="user", language=language, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.yaml"), dry_run=True, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) if expect_warning: @@ -1748,24 +1581,15 @@ def test_import_repos_unsupported_filter_warning( display_name = "CodeCommit" if service_name == "codecommit" else "GitHub" importer = MockImporter(service_name=display_name) - _run_import( + _run_import_defaults( importer, service_name=service_name, target="testuser" if service_name != "codecommit" else "", workspace=str(workspace), - mode="user", - language=None, topics=topics, min_stars=min_stars, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.yaml"), dry_run=True, - yes=True, - output_json=False, - output_ndjson=False, - color="never", ) if expect_topics_warning: @@ -1841,24 +1665,13 @@ def test_import_repos_with_shared_mode_warning( importer = MockImporter(service_name="GitLab") - _run_import( + _run_import_defaults( importer, service_name="gitlab", - target="testuser", workspace=str(workspace), mode=mode, - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.yaml"), dry_run=True, - yes=True, - output_json=False, - output_ndjson=False, - color="never", with_shared=with_shared, ) @@ -2082,24 +1895,13 @@ def test_run_import_rejects_skip_group_with_slash( workspace = tmp_path / "repos" workspace.mkdir() - result = _run_import( + result = _run_import_defaults( MockImporter(), service_name="gitlab", target="my-group", workspace=str(workspace), mode="org", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.yaml"), - dry_run=False, - yes=True, - output_json=False, - output_ndjson=False, - color="never", skip_groups=["bots/subteam"], ) @@ -2119,24 +1921,14 @@ def test_run_import_forwards_with_shared_and_skip_groups( importer = CapturingMockImporter() - _run_import( + _run_import_defaults( importer, service_name="gitlab", target="my-group", workspace=str(workspace), mode="org", - language=None, - topics=None, - min_stars=0, - include_archived=False, - include_forks=False, - limit=100, config_path_str=str(tmp_path / "config.yaml"), dry_run=True, - yes=True, - output_json=False, - output_ndjson=False, - color="never", with_shared=True, skip_groups=["bots", "archived"], )