refactor: subclass remote-build errors from CraftError #482
Conversation
Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
mr-cal
force-pushed
the
work/CRAFT-3101/remote-build-errors
branch
from
0c6c0d4
to
0056780
Compare
|
|
||
| super().__init__(brief=brief, details=details) | ||
| super().__init__(message=message, resolution=resolution) | ||
|
|
||
|
|
||
| class RemoteBuildInvalidGitRepoError(RemoteBuildError): |
There was a problem hiding this comment.
This may be out of scope for this PR, but should RemoteBuildInvalidGitRepoError be a subclass of RemoteBuildGitError?
There was a problem hiding this comment.
No, because RemoteBuildGitError is for errors that happened with git, whereas this is saying "this isn't git-able"
| 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." |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}".
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Commented before seeing Alex's response, which is better than my answer :)
tox?RemoteBuilderrors to subclassCraftErrordetailstobrief)Unblocks canonical/snapcraft#4908
(CRAFT-3101)