-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: add reviewers parameter to UpdatePullRequest #285
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
Changes from 30 commits
3234755
9db748b
a84ede9
276ac8d
53d0833
c4de80b
0e51a09
b9d2e28
9ac7250
71dcedf
5c85a09
b09f589
53e2708
7676eae
9209f5c
6f21c3f
8a6cabf
4e28ee7
432907e
0474365
20bfead
5650e1d
0674183
046f994
90eb11a
033f613
94cef70
d89e522
5ea322b
be359ea
1934e2a
c1b9a46
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 |
---|---|---|
|
@@ -151,6 +151,7 @@ func Test_UpdatePullRequest(t *testing.T) { | |
assert.Contains(t, tool.InputSchema.Properties, "state") | ||
assert.Contains(t, tool.InputSchema.Properties, "base") | ||
assert.Contains(t, tool.InputSchema.Properties, "maintainer_can_modify") | ||
assert.Contains(t, tool.InputSchema.Properties, "reviewers") | ||
assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "pullNumber"}) | ||
|
||
// Setup mock PR for success case | ||
|
@@ -173,6 +174,23 @@ func Test_UpdatePullRequest(t *testing.T) { | |
State: github.Ptr("closed"), // State updated | ||
} | ||
|
||
// Mock PR for when there are no updates but we still need a response | ||
mockNoUpdatePR := &github.PullRequest{ | ||
Number: github.Ptr(42), | ||
Title: github.Ptr("Test PR"), | ||
State: github.Ptr("open"), | ||
} | ||
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. This can be removed as well due to the referencing test not being required |
||
|
||
mockPRWithReviewers := &github.PullRequest{ | ||
Number: github.Ptr(42), | ||
Title: github.Ptr("Test PR"), | ||
State: github.Ptr("open"), | ||
RequestedReviewers: []*github.User{ | ||
{Login: github.Ptr("reviewer1")}, | ||
{Login: github.Ptr("reviewer2")}, | ||
}, | ||
} | ||
|
||
tests := []struct { | ||
name string | ||
mockedClient *http.Client | ||
|
@@ -238,6 +256,28 @@ func Test_UpdatePullRequest(t *testing.T) { | |
expectError: false, | ||
expectedPR: mockClosedPR, | ||
}, | ||
{ | ||
name: "successful PR update with reviewers", | ||
mockedClient: mock.NewMockedHTTPClient( | ||
// Mock for RequestReviewers call, returning the PR with reviewers | ||
mock.WithRequestMatch( | ||
mock.PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber, | ||
mockPRWithReviewers, | ||
), | ||
mock.WithRequestMatch( | ||
mock.GetReposPullsByOwnerByRepoByPullNumber, | ||
mockPRWithReviewers, | ||
), | ||
), | ||
requestArgs: map[string]interface{}{ | ||
"owner": "owner", | ||
"repo": "repo", | ||
"pullNumber": float64(42), | ||
"reviewers": []interface{}{"reviewer1", "reviewer2"}, | ||
}, | ||
expectError: false, | ||
expectedPR: mockPRWithReviewers, | ||
}, | ||
{ | ||
name: "successful PR update (title only)", | ||
mockedClient: mock.NewMockedHTTPClient( | ||
|
@@ -295,6 +335,32 @@ func Test_UpdatePullRequest(t *testing.T) { | |
expectError: true, | ||
expectedErrMsg: "failed to update pull request", | ||
}, | ||
{ | ||
name: "request reviewers fails", | ||
mockedClient: mock.NewMockedHTTPClient( | ||
// First it gets the PR (no fields to update) | ||
mock.WithRequestMatch( | ||
mock.GetReposPullsByOwnerByRepoByPullNumber, | ||
mockNoUpdatePR, | ||
), | ||
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. Getting the PR here is not needed |
||
// Then reviewer request fails | ||
mock.WithRequestMatchHandler( | ||
mock.PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber, | ||
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
w.WriteHeader(http.StatusUnprocessableEntity) | ||
_, _ = w.Write([]byte(`{"message": "Invalid reviewers"}`)) | ||
}), | ||
), | ||
), | ||
requestArgs: map[string]interface{}{ | ||
"owner": "owner", | ||
"repo": "repo", | ||
"pullNumber": float64(42), | ||
"reviewers": []interface{}{"invalid-user"}, | ||
}, | ||
expectError: true, | ||
expectedErrMsg: "failed to request reviewers", | ||
}, | ||
} | ||
|
||
for _, tc := range tests { | ||
|
@@ -347,6 +413,26 @@ func Test_UpdatePullRequest(t *testing.T) { | |
if tc.expectedPR.MaintainerCanModify != nil { | ||
assert.Equal(t, *tc.expectedPR.MaintainerCanModify, *returnedPR.MaintainerCanModify) | ||
} | ||
|
||
// Check reviewers if they exist in the expected PR | ||
if len(tc.expectedPR.RequestedReviewers) > 0 { | ||
assert.NotNil(t, returnedPR.RequestedReviewers) | ||
assert.Equal(t, len(tc.expectedPR.RequestedReviewers), len(returnedPR.RequestedReviewers)) | ||
|
||
// Create maps of reviewer logins for easy comparison | ||
expectedReviewers := make(map[string]bool) | ||
for _, reviewer := range tc.expectedPR.RequestedReviewers { | ||
expectedReviewers[*reviewer.Login] = true | ||
} | ||
|
||
actualReviewers := make(map[string]bool) | ||
for _, reviewer := range returnedPR.RequestedReviewers { | ||
actualReviewers[*reviewer.Login] = true | ||
} | ||
|
||
// Compare the maps | ||
assert.Equal(t, expectedReviewers, actualReviewers) | ||
} | ||
}) | ||
} | ||
} | ||
|
@@ -529,6 +615,22 @@ func Test_UpdatePullRequest_Draft(t *testing.T) { | |
err = json.Unmarshal([]byte(textContent.Text), &returnedPR) | ||
require.NoError(t, err) | ||
assert.Equal(t, *tc.expectedPR.Number, *returnedPR.Number) | ||
if tc.expectedPR.Title != nil { | ||
assert.Equal(t, *tc.expectedPR.Title, *returnedPR.Title) | ||
} | ||
if tc.expectedPR.Body != nil { | ||
assert.Equal(t, *tc.expectedPR.Body, *returnedPR.Body) | ||
} | ||
if tc.expectedPR.State != nil { | ||
assert.Equal(t, *tc.expectedPR.State, *returnedPR.State) | ||
} | ||
if tc.expectedPR.Base != nil && tc.expectedPR.Base.Ref != nil { | ||
assert.NotNil(t, returnedPR.Base) | ||
assert.Equal(t, *tc.expectedPR.Base.Ref, *returnedPR.Base.Ref) | ||
} | ||
if tc.expectedPR.MaintainerCanModify != nil { | ||
assert.Equal(t, *tc.expectedPR.MaintainerCanModify, *returnedPR.MaintainerCanModify) | ||
} | ||
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. This change is out of scope for this PR |
||
if tc.expectedPR.Draft != nil { | ||
assert.Equal(t, *tc.expectedPR.Draft, *returnedPR.Draft) | ||
} | ||
|
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.
We could simplify it to
!(restUpdateNeeded || draftProvided || len(reviewers) != 0)