-
Notifications
You must be signed in to change notification settings - Fork 12
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,25 +15,21 @@ | |
from craft_application.remote import errors | ||
|
||
|
||
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." | ||
Comment on lines
+18
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commented before seeing Alex's response, which is better than my answer :) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
# Copyright 2024 Canonical Ltd. | ||
# | ||
# This program is free software: you can redistribute it and/or modify | ||
# it under the terms of the GNU Lesser General Public License version 3 as | ||
# published by the Free Software Foundation. | ||
# | ||
# This program is distributed in the hope that it will be useful, | ||
# but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
# GNU Lesser General Public License for more details. | ||
# | ||
# You should have received a copy of the GNU Lesser General Public License | ||
# along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
"""Remote-build git tests.""" | ||
|
||
import pytest | ||
from craft_application.git import GitType | ||
from craft_application.remote import errors, git | ||
|
||
|
||
@pytest.fixture | ||
def mock_get_git_repo_type(mocker): | ||
return mocker.patch("craft_application.remote.git.get_git_repo_type") | ||
|
||
|
||
def test_git_normal(tmp_path, mock_get_git_repo_type): | ||
"""No-op for a normal git repo.""" | ||
mock_get_git_repo_type.return_value = GitType.NORMAL | ||
|
||
assert git.check_git_repo_for_remote_build(tmp_path) is None | ||
|
||
|
||
def test_git_invalid_error(tmp_path, mock_get_git_repo_type): | ||
"""Raise an error for invalid git repos.""" | ||
mock_get_git_repo_type.return_value = GitType.INVALID | ||
|
||
with pytest.raises(errors.RemoteBuildInvalidGitRepoError) as err: | ||
git.check_git_repo_for_remote_build(tmp_path) | ||
|
||
assert str(err.value) == f"Could not find a git repository in {str(tmp_path)!r}" | ||
assert ( | ||
err.value.resolution == "Initialize a git repository in the project directory" | ||
) | ||
|
||
|
||
def test_git_shallow_clone_error(tmp_path, mock_get_git_repo_type): | ||
"""Raise an error for shallowly cloned repos.""" | ||
mock_get_git_repo_type.return_value = GitType.SHALLOW | ||
|
||
with pytest.raises(errors.RemoteBuildInvalidGitRepoError) as err: | ||
git.check_git_repo_for_remote_build(tmp_path) | ||
|
||
assert ( | ||
str(err.value) == "Remote builds for shallow cloned git repos are not supported" | ||
) | ||
assert err.value.resolution == "Make a non-shallow clone of the repository" |
There was a problem hiding this comment.
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 ofRemoteBuildGitError
?There was a problem hiding this comment.
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"