From ee779cec4d018181dc1d95b8717f953c966709a5 Mon Sep 17 00:00:00 2001 From: zvecr Date: Wed, 1 Jul 2026 04:25:50 +0100 Subject: [PATCH] Fix git_clone_fork exit code regression --- qmk_cli/subcommands/setup.py | 19 ++++++---- test/subcommands/test_setup.py | 69 ++++++++++++++++++++++++++++------ 2 files changed, 68 insertions(+), 20 deletions(-) diff --git a/qmk_cli/subcommands/setup.py b/qmk_cli/subcommands/setup.py index 56816d8..be9aa97 100644 --- a/qmk_cli/subcommands/setup.py +++ b/qmk_cli/subcommands/setup.py @@ -23,7 +23,7 @@ DEFAULT_BRANCH = 'master' @cli.argument('-H', '--home', arg_only=True, default=Path(os.environ['QMK_HOME']), type=AbsPath, help='The location for QMK Firmware. Default: %s' % os.environ['QMK_HOME']) @cli.argument('fork', arg_only=True, default=DEFAULT_FORK, nargs='?', help='The qmk_firmware fork to clone. Default: %s' % DEFAULT_FORK) @cli.subcommand('Setup your computer for qmk_firmware.') -def setup(cli): +def setup(cli): # noqa: C901 """Guide the user through setting up their QMK environment. """ clone_prompt = 'Would you like to clone {fg_cyan}%s{fg_reset} to {fg_cyan}%s{fg_reset}?' % (cli.args.fork, shlex.quote(str(cli.args.home))) @@ -32,7 +32,7 @@ def setup(cli): # Sanity checks if cli.args.yes and cli.args.no: cli.log.error("Can't use both --yes and --no at the same time.") - exit(1) + return False # Check on qmk_firmware # If it exists, ask the user what to do with it @@ -50,18 +50,20 @@ def setup(cli): found_action = choice(found_prompt, options=found_options, default=2) if found_action == f"Delete and reclone {cli.args.fork}": if not yesno(delete_confirm, default=False): - exit(1) + return False - git_clone_fork(cli.args.home, cli.args.baseurl, cli.args.fork, cli.args.branch, force=True) + if not git_clone_fork(cli.args.home, cli.args.baseurl, cli.args.fork, cli.args.branch, force=True): + return False elif found_action == "Delete and clone a different fork": fork_name = question("Enter the name of the fork:", default=cli.args.fork) branch_name = question("Enter the branch name to clone:", default=cli.args.branch) if not yesno(delete_confirm, default=False): - exit(1) + return False - git_clone_fork(cli.args.home, cli.args.baseurl, fork_name, branch_name, force=True) + if not git_clone_fork(cli.args.home, cli.args.baseurl, fork_name, branch_name, force=True): + return False # Exists (but not an empty dir) elif cli.args.home.exists() and any(cli.args.home.iterdir()): @@ -71,12 +73,13 @@ def setup(cli): cli.log.warning('Warning: %s does not end in "qmk_firmware". Did you mean to use "--home %s/qmk_firmware"?' % (path_str, path_str)) cli.log.error("Path '%s' exists but is not a qmk_firmware clone!", path_str) - exit(1) + return False else: cli.log.error('Could not find qmk_firmware!') if yesno(clone_prompt): - git_clone_fork(cli.args.home, cli.args.baseurl, cli.args.fork, cli.args.branch) + if not git_clone_fork(cli.args.home, cli.args.baseurl, cli.args.fork, cli.args.branch): + return False else: cli.log.warning('Not cloning qmk_firmware due to user input or --no flag.') diff --git a/test/subcommands/test_setup.py b/test/subcommands/test_setup.py index 417cd25..bd50009 100644 --- a/test/subcommands/test_setup.py +++ b/test/subcommands/test_setup.py @@ -49,10 +49,9 @@ def test_setup_both_yes_no_invalid(subcommand, mock_cli): type(mock_cli.args).yes = PropertyMock(return_value=True) type(mock_cli.args).no = PropertyMock(return_value=True) - with pytest.raises(SystemExit) as e: - subcommand.setup(mock_cli) + ret = subcommand.setup(mock_cli) - assert e.value.code == 1 + assert ret is False def test_setup_reclone(subcommand, mock_cli, is_qmk_firmware, temp_directory, yesno, choice, git_clone_fork): @@ -70,6 +69,23 @@ def test_setup_reclone(subcommand, mock_cli, is_qmk_firmware, temp_directory, ye git_clone_fork.assert_called_once() +def test_setup_reclone_failed(subcommand, mock_cli, is_qmk_firmware, temp_directory, yesno, choice, git_clone_fork): + type(mock_cli.args).yes = PropertyMock(return_value=False) + type(mock_cli.args).no = PropertyMock(return_value=False) + type(mock_cli.args).home = PropertyMock(return_value=temp_directory) + type(mock_cli.args).fork = 'asdf' + choice.return_value = 'Delete and reclone asdf' + yesno.return_value = True + git_clone_fork.return_value = False + + ret = subcommand.setup(mock_cli) + + yesno.assert_called_once() + assert 'This will delete your current qmk_firmware directory.' in yesno.call_args.args[0] + git_clone_fork.assert_called_once() + assert ret is False + + def test_setup_reclone_no(subcommand, mock_cli, is_qmk_firmware, temp_directory, yesno, choice, git_clone_fork): type(mock_cli.args).yes = PropertyMock(return_value=False) type(mock_cli.args).no = PropertyMock(return_value=False) @@ -78,10 +94,9 @@ def test_setup_reclone_no(subcommand, mock_cli, is_qmk_firmware, temp_directory, choice.return_value = 'Delete and reclone asdf' yesno.return_value = False - with pytest.raises(SystemExit) as e: - subcommand.setup(mock_cli) + ret = subcommand.setup(mock_cli) - assert e.value.code == 1 + assert ret is False git_clone_fork.assert_not_called() @@ -99,6 +114,22 @@ def test_setup_clone_diff_fork(subcommand, mock_cli, is_qmk_firmware, temp_direc git_clone_fork.assert_called_once() +def test_setup_clone_diff_fork_failed(subcommand, mock_cli, is_qmk_firmware, temp_directory, yesno, choice, question, git_clone_fork): + type(mock_cli.args).yes = PropertyMock(return_value=False) + type(mock_cli.args).no = PropertyMock(return_value=False) + type(mock_cli.args).home = PropertyMock(return_value=temp_directory) + choice.return_value = 'Delete and clone a different fork' + yesno.return_value = True + git_clone_fork.return_value = False + + ret = subcommand.setup(mock_cli) + + yesno.assert_called_once() + assert 'This will delete your current qmk_firmware directory.' in yesno.call_args.args[0] + git_clone_fork.assert_called_once() + assert ret is False + + def test_setup_clone_diff_fork_no(subcommand, mock_cli, is_qmk_firmware, temp_directory, yesno, choice, question, git_clone_fork): type(mock_cli.args).yes = PropertyMock(return_value=False) type(mock_cli.args).no = PropertyMock(return_value=False) @@ -106,10 +137,9 @@ def test_setup_clone_diff_fork_no(subcommand, mock_cli, is_qmk_firmware, temp_di choice.return_value = 'Delete and clone a different fork' yesno.return_value = False - with pytest.raises(SystemExit) as e: - subcommand.setup(mock_cli) + ret = subcommand.setup(mock_cli) - assert e.value.code == 1 + assert ret is False git_clone_fork.assert_not_called() @@ -120,10 +150,9 @@ def test_setup_home_exists_not_empty(subcommand, mock_cli, is_qmk_firmware, temp is_qmk_firmware.return_value = False (temp_directory / 'asdf').touch() - with pytest.raises(SystemExit) as e: - subcommand.setup(mock_cli) + ret = subcommand.setup(mock_cli) - assert e.value.code == 1 + assert ret is False def test_setup_missing(subcommand, mock_cli, is_qmk_firmware, temp_directory, yesno, git_clone_fork): @@ -140,6 +169,22 @@ def test_setup_missing(subcommand, mock_cli, is_qmk_firmware, temp_directory, ye git_clone_fork.assert_called_once() +def test_setup_missing_failed(subcommand, mock_cli, is_qmk_firmware, temp_directory, yesno, git_clone_fork): + type(mock_cli.args).yes = PropertyMock(return_value=False) + type(mock_cli.args).no = PropertyMock(return_value=False) + type(mock_cli.args).home = PropertyMock(return_value=temp_directory) + is_qmk_firmware.return_value = False + yesno.return_value = True + git_clone_fork.return_value = False + + ret = subcommand.setup(mock_cli) + + yesno.assert_called_once() + assert 'Would you like to clone' in yesno.call_args.args[0] + git_clone_fork.assert_called_once() + assert ret is False + + def test_setup_missing_no(subcommand, mock_cli, is_qmk_firmware, temp_directory, yesno, git_clone_fork): type(mock_cli.args).yes = PropertyMock(return_value=False) type(mock_cli.args).no = PropertyMock(return_value=False)