-
Notifications
You must be signed in to change notification settings - Fork 81
Allow resubmits for builds that fails during SRPM imports #4097
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
base: main
Are you sure you want to change the base?
Allow resubmits for builds that fails during SRPM imports #4097
Conversation
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.
Code Review
This pull request correctly implements the logic to allow resubmission of builds that fail during the SRPM import phase. The changes in the Build model and the backend view are logical and are accompanied by new tests. I have provided a couple of suggestions to improve code conciseness and test coverage.
| if self.source_status == StatusEnum("succeeded"): | ||
| return True | ||
| if (self.source_status == StatusEnum("failed") and | ||
| self.fail_type == FailTypeEnum("srpm_import_failed")): | ||
| return True | ||
| return False |
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.
The logic for determining if a build is repeatable can be simplified into a single boolean expression. This improves conciseness and readability.
return (self.source_status == StatusEnum("succeeded") or
(self.source_status == StatusEnum("failed") and
self.fail_type == FailTypeEnum("srpm_import_failed")))| def test_repeatable_succeeded(self, f_users, f_coprs, f_builds, f_db): | ||
| """ | ||
| Test that builds with succeeded source_status are repeatable. | ||
| """ | ||
| self.b1.source_status = StatusEnum("succeeded") | ||
| assert self.b1.repeatable | ||
|
|
||
| def test_repeatable_import_failed(self, f_users, f_coprs, f_builds, f_db): | ||
| """ | ||
| Test that builds which failed during import phase are repeatable. | ||
| """ | ||
| self.b1.source_status = StatusEnum("failed") | ||
| self.b1.fail_type = FailTypeEnum("srpm_import_failed") | ||
| assert self.b1.repeatable |
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.
The new tests cover the cases where a build should be repeatable. It would be beneficial to also add test cases for when a build is not repeatable to ensure the logic is fully covered. For example, when a build fails for a different reason or is in a different state.
def test_repeatable_succeeded(self, f_users, f_coprs, f_builds, f_db):
"""
Test that builds with succeeded source_status are repeatable.
"""
self.b1.source_status = StatusEnum("succeeded")
assert self.b1.repeatable
def test_repeatable_import_failed(self, f_users, f_coprs, f_builds, f_db):
"""
Test that builds which failed during import phase are repeatable.
"""
self.b1.source_status = StatusEnum("failed")
self.b1.fail_type = FailTypeEnum("srpm_import_failed")
assert self.b1.repeatable
def test_not_repeatable(self, f_users, f_coprs, f_builds, f_db):
"""
Test that builds that failed for other reasons are not repeatable.
"""
# failed for another reason
self.b1.source_status = StatusEnum("failed")
self.b1.fail_type = FailTypeEnum("unknown_error")
assert not self.b1.repeatable
# still running
self.b1.source_status = StatusEnum("running")
self.b1.fail_type = FailTypeEnum("unset")
assert not self.b1.repeatable
# just failed, but fail_type is not set
self.b1.source_status = StatusEnum("failed")
self.b1.fail_type = FailTypeEnum("unset")
assert not self.b1.repeatable274b2eb to
9706ba4
Compare
Pull Request validationFailed🔴 Review - Missing review from a member (2 required) Success🟢 CI - All checks have passed |
Fixes #2844
Make builds repeatable when SRPM build is successful but failed to import to dist-git.