Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: override _run_inner instead of run to catch exceptions #5035

Merged
merged 3 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion requirements-devel.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ click==8.1.7
codespell==2.3.0
colorama==0.4.6
coverage==7.6.1
craft-application==4.1.2
craft-application==4.2.2
lengau marked this conversation as resolved.
Show resolved Hide resolved
craft-archives==2.0.0
craft-cli==2.7.0
craft-grammar==2.0.0
Expand Down
2 changes: 1 addition & 1 deletion requirements-docs.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ chardet==5.2.0
charset-normalizer==3.3.2
click==8.1.7
colorama==0.4.6
craft-application==4.1.2
craft-application==4.2.2
lengau marked this conversation as resolved.
Show resolved Hide resolved
craft-archives==2.0.0
craft-cli==2.7.0
craft-grammar==2.0.0
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ cffi==1.17.1
chardet==5.2.0
charset-normalizer==3.3.2
click==8.1.7
craft-application==4.1.2
craft-application==4.2.2
lengau marked this conversation as resolved.
Show resolved Hide resolved
craft-archives==2.0.0
craft-cli==2.7.0
craft-grammar==2.0.0
Expand Down
10 changes: 4 additions & 6 deletions snapcraft/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,13 @@ def _pre_run(self, dispatcher: craft_cli.Dispatcher) -> None:
super()._pre_run(dispatcher)

@override
def run(self) -> int:
def _run_inner(self) -> int:
try:
return_code = super().run()
return_code = super()._run_inner()
except craft_store.errors.NoKeyringError as err:
self._emit_error(
craft_cli.errors.CraftError(
f"craft-store error: {err}",
f"{err}",
resolution=(
"Ensure the keyring is working or "
f"{store.constants.ENVIRONMENT_STORE_CREDENTIALS} "
Expand All @@ -227,9 +227,7 @@ def run(self) -> int:
return_code = 1
except craft_store.errors.CraftStoreError as err:
self._emit_error(
craft_cli.errors.CraftError(
f"craft-store error: {err}", resolution=err.resolution
),
craft_cli.errors.CraftError(f"{err}", resolution=err.resolution),
cause=err,
)
return_code = 1
Expand Down
3 changes: 2 additions & 1 deletion tests/unit/commands/test_lifecycle.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ def test_snap_command_fallback(tmp_path, emitter, mocker, fake_services):
)


def test_core24_try_command(tmp_path, mocker, fake_services):
@pytest.mark.usefixtures("emitter")
def test_core24_try_command(tmp_path, fake_services):
parsed_args = argparse.Namespace(parts=[], output=tmp_path)
cmd = lifecycle.TryCommand({"app": APP_METADATA, "services": fake_services})

Expand Down
45 changes: 18 additions & 27 deletions tests/unit/commands/test_remote.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@

# remote-build control logic may check if the working dir is a git repo,
# so execute all tests inside a test directory
pytestmark = pytest.mark.usefixtures("new_dir")
# The service also emits
pytestmark = pytest.mark.usefixtures("new_dir", "emitter")


@pytest.fixture()
Expand Down Expand Up @@ -163,7 +164,7 @@ def test_no_confirmation_for_private_project(


@pytest.mark.parametrize("base", const.CURRENT_BASES)
@pytest.mark.usefixtures("mock_argv")
@pytest.mark.usefixtures("mock_argv", "emitter")
lengau marked this conversation as resolved.
Show resolved Hide resolved
def test_command_user_confirms_upload(
snapcraft_yaml, base, mock_confirm, fake_services
):
Expand All @@ -183,7 +184,7 @@ def test_command_user_confirms_upload(


@pytest.mark.parametrize("base", const.CURRENT_BASES)
@pytest.mark.usefixtures("mock_argv", "fake_services")
@pytest.mark.usefixtures("mock_argv", "emitter", "fake_services")
def test_command_user_denies_upload(
capsys,
snapcraft_yaml,
Expand All @@ -206,7 +207,7 @@ def test_command_user_denies_upload(


@pytest.mark.parametrize("base", const.CURRENT_BASES)
@pytest.mark.usefixtures("mock_argv", "fake_services")
@pytest.mark.usefixtures("mock_argv", "emitter", "fake_services")
def test_command_accept_upload(
mocker, snapcraft_yaml, base, mock_confirm, mock_run_remote_build
):
Expand All @@ -224,7 +225,9 @@ def test_command_accept_upload(


@pytest.mark.parametrize("base", const.CURRENT_BASES)
@pytest.mark.usefixtures("mock_argv", "mock_confirm", "fake_services", "fake_sudo")
@pytest.mark.usefixtures(
"mock_argv", "mock_confirm", "emitter", "fake_services", "fake_sudo"
)
def test_remote_build_sudo_warns(emitter, snapcraft_yaml, base, mock_run_remote_build):
"Check if a warning is shown when snapcraft is run with sudo."
snapcraft_yaml_dict = {"base": base, "build-base": "devel", "grade": "devel"}
Expand All @@ -240,8 +243,8 @@ def test_remote_build_sudo_warns(emitter, snapcraft_yaml, base, mock_run_remote_


@pytest.mark.parametrize("base", const.CURRENT_BASES)
@pytest.mark.usefixtures("mock_argv", "mock_confirm", "fake_services")
def test_launchpad_timeout_default(mocker, snapcraft_yaml, base, fake_services):
@pytest.mark.usefixtures("mock_argv", "mock_confirm", "emitter", "fake_services")
def test_launchpad_timeout_default(mocker, snapcraft_yaml, base):
"""Check if no timeout is set by default."""
snapcraft_yaml_dict = {"base": base, "build-base": "devel", "grade": "devel"}
snapcraft_yaml(**snapcraft_yaml_dict)
Expand All @@ -256,8 +259,8 @@ def test_launchpad_timeout_default(mocker, snapcraft_yaml, base, fake_services):


@pytest.mark.parametrize("base", const.CURRENT_BASES)
@pytest.mark.usefixtures("mock_argv", "mock_confirm", "fake_services")
def test_launchpad_timeout(mocker, snapcraft_yaml, base, fake_services):
@pytest.mark.usefixtures("mock_argv", "mock_confirm", "emitter", "fake_services")
def test_launchpad_timeout(mocker, snapcraft_yaml, base):
"""Set the timeout for the remote builder."""
mocker.patch.object(
sys, "argv", ["snapcraft", "remote-build", "--launchpad-timeout", "100"]
Expand All @@ -281,7 +284,7 @@ def test_launchpad_timeout(mocker, snapcraft_yaml, base, fake_services):


@pytest.mark.parametrize("base", const.CURRENT_BASES)
@pytest.mark.usefixtures("mock_argv", "mock_confirm", "fake_services")
@pytest.mark.usefixtures("mock_argv", "mock_confirm", "emitter", "fake_services")
def test_run_core22_and_later(snapcraft_yaml, base, mock_remote_build_run):
"""Bases that are core22 and later will use craft-application remote-build."""
snapcraft_yaml_dict = {"base": base, "build-base": "devel", "grade": "devel"}
Expand Down Expand Up @@ -309,15 +312,8 @@ def test_run_core20(


@pytest.mark.parametrize("base", const.CURRENT_BASES)
@pytest.mark.usefixtures("mock_confirm", "mock_argv")
def test_run_in_repo_newer_than_core22(
emitter,
mocker,
snapcraft_yaml,
base,
new_dir,
fake_services,
):
@pytest.mark.usefixtures("mock_confirm", "mock_argv", "emitter", "fake_services")
def test_run_in_repo_newer_than_core22(mocker, snapcraft_yaml, base, new_dir):
"""Bases newer than core22 run craft-application remote-build regardless of being in a repo."""
# initialize a git repo
GitRepo(new_dir)
Expand All @@ -337,15 +333,10 @@ def test_run_in_repo_newer_than_core22(
@pytest.mark.usefixtures(
"mock_confirm",
"mock_argv",
"mock_remote_builder_start_builds",
"fake_services",
)
def test_run_in_shallow_repo_unsupported(
capsys,
new_dir,
snapcraft_yaml,
base,
mock_remote_builder_start_builds,
fake_services,
):
def test_run_in_shallow_repo_unsupported(capsys, new_dir, snapcraft_yaml, base):
"""devel / core24 and newer bases run new remote-build in a shallow git repo."""
root_path = Path(new_dir)
snapcraft_yaml_dict = {"base": base, "build-base": "devel", "grade": "devel"}
Expand Down
8 changes: 4 additions & 4 deletions tests/unit/test_application.py
Original file line number Diff line number Diff line change
Expand Up @@ -665,20 +665,20 @@ def test_known_core24(snapcraft_yaml, base, build_base, is_known_core24):
)
def test_store_error(mocker, capsys, message, resolution, expected_message):
mocker.patch(
"snapcraft.application.Application.run",
"snapcraft.application.Application._run_inner",
side_effect=craft_store.errors.CraftStoreError(message, resolution=resolution),
)

return_code = application.main()

assert return_code == 1
_, err = capsys.readouterr()
assert f"craft-store error: {expected_message}" in err
assert expected_message in err


def test_store_key_error(mocker, capsys):
mocker.patch(
"snapcraft.application.Application.run",
"snapcraft.application.Application._run_inner",
side_effect=craft_store.errors.NoKeyringError(),
)

Expand All @@ -692,7 +692,7 @@ def test_store_key_error(mocker, capsys):
# pylint: disable=[line-too-long]
dedent(
"""\
craft-store error: No keyring found to store or retrieve credentials from.
No keyring found to store or retrieve credentials from.
Recommended resolution: Ensure the keyring is working or SNAPCRAFT_STORE_CREDENTIALS is correctly exported into the environment
For more information, check out: https://snapcraft.io/docs/snapcraft-authentication
"""
Expand Down
Loading