add message on pr checks returns no CI with non-empty PR#2279
add message on pr checks returns no CI with non-empty PR#2279
pr checks returns no CI with non-empty PR#2279Conversation
pkg/cmd/pr/checks/checks_test.go
Outdated
| wantOut string | ||
| nontty bool | ||
| wantErr bool | ||
| throw bool |
There was a problem hiding this comment.
I think we could rename wantErr to unsuccessfulChecks and throw to wantErr to make the purpose of them more clear.
pkg/cmd/pr/checks/checks_test.go
Outdated
| if err != nil && tt.throw { | ||
| assert.Equal(t, tt.wantOut, err.Error()) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
If we make the naming change above this could be changed to
if tt.wantErr {
assert.Equal(t, tt.wantOut, err.Error())
} else if tt.unsuccessfulChecks {
assert.Equal(t, "SilentError", err.Error())
} else {
assert.NoError(t, err)
} This seems a bit more clear to me.
|
would |
samcoe
left a comment
samcoe
left a comment
There was a problem hiding this comment.
Thanks for making the requested changes, this looks great!
pkg/cmd/pr/checks/checks.go
Outdated
| rollup := pr.Commits.Nodes[0].Commit.StatusCheckRollup.Contexts.Nodes | ||
| if len(rollup) == 0 { | ||
| return nil | ||
| return fmt.Errorf("no checks are set up for this repository") |
There was a problem hiding this comment.
I like that the command is exiting with a nonzero status now! However, I do not think that this error message will always be accurate:
-
The branch for the PR might not report any checks if the branch was just created, so before any Checks nor Statuses got registered by external services yet. Re-running the command a few seconds later could show that there are now checks registered as “pending”.
-
The branch for the PR might not have associated checks because the base branch for the PR is not configured to run checks. However, a PR based on a different branch might have checks. So checks are often dependent on the particular branch settings, not just per-repo settings.
So, I think a better error message should be something more vague, like no checks reported on the 'foo' branch. I'd also like to hear if there are better ideas? 🙏
Should we also exit with nonzero status in the above len(pr.Commits.Nodes) condition where the PR does not appear to have any commits?
There was a problem hiding this comment.
I agree with you about changing the message according to the branch, that make more sense.
And for the empty commits case, maybe no commit found on the pull request would just be ok
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
Thank you!
I've pushed a commit that simplifies the test re: wantErr handling. 🙇
mislav
force-pushed
the
1818-pr-checks-empty
branch
from
1836cdb to
6b953f2
Compare
5 participants
fixes #1818