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

refactor: subclass remote-build errors from CraftError #482

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

mr-cal
Copy link
Contributor

@mr-cal mr-cal commented Sep 19, 2024

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

  1. Refactor RemoteBuild errors to subclass CraftError
  2. Slight changes to how remote build error are presented (moving information from details to brief)
  3. Release notes for 4.2.4

Unblocks canonical/snapcraft#4908
(CRAFT-3101)

Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
@mr-cal mr-cal changed the base branch from main to hotfix/4.2 September 19, 2024 19:44
@mr-cal mr-cal added the rebase label Sep 19, 2024
Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
@mr-cal mr-cal force-pushed the work/CRAFT-3101/remote-build-errors branch from 0c6c0d4 to 0056780 Compare September 19, 2024 19:45
@mr-cal mr-cal requested review from lengau, a team and dariuszd21 September 19, 2024 19:47
@lengau lengau requested a review from a team September 20, 2024 12:06

super().__init__(brief=brief, details=details)
super().__init__(message=message, resolution=resolution)


class RemoteBuildInvalidGitRepoError(RemoteBuildError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be out of scope for this PR, but should RemoteBuildInvalidGitRepoError be a subclass of RemoteBuildGitError?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because RemoteBuildGitError is for errors that happened with git, whereas this is saying "this isn't git-able"

Comment on lines +18 to +35
def test_git_error():
"""Test RemoteBuildGitError."""
error = errors.RemoteBuildGitError(message="failed to push some refs to 'unknown'")

assert (
str(error) == "Git operation failed with: failed to push some refs to 'unknown'"
)


def test_unsupported_architecture_error():
"""Test UnsupportedArchitectureError."""
error = errors.UnsupportedArchitectureError(architectures=["amd64", "arm64"])

assert str(error) == (
"Architecture not supported by the remote builder.\nThe following "
"architectures are not supported by the remote builder: ['amd64', 'arm64'].\n"
"Please remove them from the architecture list and try again."
)
assert repr(error) == (
"UnsupportedArchitectureError(brief='Architecture not supported by the remote "
"builder.', details=\"The following architectures are not supported by the "
"remote builder: ['amd64', 'arm64'].\\nPlease remove them from the "
'architecture list and try again.")'
)

assert error.brief == "Architecture not supported by the remote builder."
assert error.details == (
"The following architectures are not supported by the remote builder: "
"['amd64', 'arm64'].\nPlease remove them from the architecture list and "
"try again."
"'amd64' and 'arm64'."
)
assert error.resolution == "Remove them from the architecture list and try again."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also perhaps out of scope, but I'd argue that these tests aren't really all that useful. Feels like checking the code coverage box, but doesn't really test any logic or flow, just something that will need updating when the string changes in the source.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic tested here is that we have our formatting strings correct. It prevents regressions where one may change f"my format {string}" to `"my format {string}".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we do and do not test this kind of stuff throughout our projects, so I'm not sure what the team stance is.

FWIW, this won't increase code coverage unless there are no unit tests that raise the errors.

Given how many UX regressions we've experienced, I slightly lean towards doing tests like this to ensure the custom formatting in these errors will look as expected.

If someone on the team is interested in a writeup of our team's unit testing practices, that would be very welcome!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented before seeing Alex's response, which is better than my answer :)

@lengau lengau merged commit cb61e69 into hotfix/4.2 Sep 20, 2024
7 checks passed
@lengau lengau deleted the work/CRAFT-3101/remote-build-errors branch September 20, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants