diff --git a/backend/plugins/github/e2e/pr_unfinished_test.go b/backend/plugins/github/e2e/pr_unfinished_test.go new file mode 100644 index 00000000000..f64d7af8da4 --- /dev/null +++ b/backend/plugins/github/e2e/pr_unfinished_test.go @@ -0,0 +1,70 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package e2e + +import ( + "sort" + "testing" + + "github.com/apache/devlake/helpers/e2ehelper" + "github.com/apache/devlake/plugins/github/impl" + "github.com/apache/devlake/plugins/github/models" + "github.com/apache/devlake/plugins/github/tasks" + "github.com/stretchr/testify/assert" +) + +// TestGithubUnfinishedPullRequests pins the set of pull requests the "Collect Pull Requests" +// detail collector re-collects on an incremental run. The seeded table covers every +// combination that decides it, and the repo/connection of another scope. +// +// Pull request 6 is the regression guard: it is closed, was never merged and its details +// were already collected, so there is nothing left to collect for it. The previous +// condition (`state != 'closed' OR merged = false`) kept it in the set on every run, +// forever, which cost one request per closed and unmerged pull request per run (#9240). +func TestGithubUnfinishedPullRequests(t *testing.T) { + var githubPlugin impl.Github + dataflowTester := e2ehelper.NewDataFlowTester(t, "github", githubPlugin) + + dataflowTester.ImportCsvIntoTabler( + "./raw_tables/_tool_github_pull_requests_unfinished.csv", + &models.GithubPullRequest{}, + ) + + var pullRequests []models.GithubPullRequest + assert.Nil(t, dataflowTester.Dal.All( + &pullRequests, + tasks.UnfinishedPullRequestsClauses(134018330, 1)..., + )) + + numbers := make([]int, len(pullRequests)) + for i, pullRequest := range pullRequests { + numbers[i] = pullRequest.Number + } + sort.Ints(numbers) + + assert.Equal(t, []int{ + 1, // open, details never collected + 2, // open, details already collected + 4, // closed and merged on GitHub, `merged` not collected yet + 5, // closed, never merged, details never collected + }, numbers) + assert.NotContains(t, numbers, 6, "closed and unmerged pull requests with collected details must not be re-collected") + assert.NotContains(t, numbers, 3, "closed and merged pull requests must not be re-collected") + assert.NotContains(t, numbers, 7, "pull requests of another repo must not be collected") + assert.NotContains(t, numbers, 8, "pull requests of another connection must not be collected") +} diff --git a/backend/plugins/github/e2e/raw_tables/_tool_github_pull_requests_unfinished.csv b/backend/plugins/github/e2e/raw_tables/_tool_github_pull_requests_unfinished.csv new file mode 100644 index 00000000000..00093876ad3 --- /dev/null +++ b/backend/plugins/github/e2e/raw_tables/_tool_github_pull_requests_unfinished.csv @@ -0,0 +1,9 @@ +connection_id,github_id,repo_id,number,state,commits,merged,merged_at,github_created_at,github_updated_at,closed_at +1,101,134018330,1,open,0,0,,2018-07-25T08:19:30.000+00:00,2018-07-25T08:19:30.000+00:00, +1,102,134018330,2,open,3,0,,2018-07-25T08:19:30.000+00:00,2018-07-26T08:19:30.000+00:00, +1,103,134018330,3,closed,7,1,2018-07-26T02:32:41.000+00:00,2018-07-25T08:19:30.000+00:00,2018-07-26T02:32:41.000+00:00,2018-07-26T02:32:41.000+00:00 +1,104,134018330,4,closed,0,0,2018-07-26T02:32:41.000+00:00,2018-07-25T08:19:30.000+00:00,2018-07-26T02:32:41.000+00:00,2018-07-26T02:32:41.000+00:00 +1,105,134018330,5,closed,0,0,,2018-07-25T08:19:30.000+00:00,2018-07-26T02:32:41.000+00:00,2018-07-26T02:32:41.000+00:00 +1,106,134018330,6,closed,2,0,,2018-07-25T08:19:30.000+00:00,2018-07-26T02:32:41.000+00:00,2018-07-26T02:32:41.000+00:00 +1,107,999999999,7,open,0,0,,2018-07-25T08:19:30.000+00:00,2018-07-25T08:19:30.000+00:00, +2,108,134018330,8,open,0,0,,2018-07-25T08:19:30.000+00:00,2018-07-25T08:19:30.000+00:00, diff --git a/backend/plugins/github/tasks/pr_collector.go b/backend/plugins/github/tasks/pr_collector.go index 9c90470ec52..71a12a707ca 100644 --- a/backend/plugins/github/tasks/pr_collector.go +++ b/backend/plugins/github/tasks/pr_collector.go @@ -105,14 +105,8 @@ func CollectApiPullRequests(taskCtx plugin.SubTaskContext) errors.Error { }, CollectUnfinishedDetails: &helper.FinalizableApiCollectorDetailArgs{ BuildInputIterator: func() (helper.Iterator, errors.Error) { - // select pull id from database cursor, err := db.Cursor( - dal.Select("number"), - dal.From(&models.GithubPullRequest{}), - dal.Where( - "repo_id = ? AND connection_id = ? AND (state != 'closed' OR merged = false)", - data.Options.GithubId, data.Options.ConnectionId, - ), + UnfinishedPullRequestsClauses(data.Options.GithubId, data.Options.ConnectionId)..., ) if err != nil { return nil, err @@ -140,3 +134,23 @@ func CollectApiPullRequests(taskCtx plugin.SubTaskContext) errors.Error { return collector.Execute() } + +// UnfinishedPullRequestsClauses selects the pull requests whose details the "Collect Pull +// Requests" subtask re-collects one by one on an incremental run: the ones that are still +// open, plus the ones whose detail-only fields have never been collected. +// +// `repos/{owner}/{repo}/pulls` does not return `merged`, `commits`, `additions`, +// `deletions`, `review_comments` or `merged_by`, so a pull request that was only ever seen +// in a list page is stored with `commits = 0`. Every pull request has at least one commit, +// which makes `commits = 0` the marker for "details not collected yet", and the condition +// stops matching as soon as the details are in. +func UnfinishedPullRequestsClauses(repoId int, connectionId uint64) []dal.Clause { + return []dal.Clause{ + dal.Select("number"), + dal.From(&models.GithubPullRequest{}), + dal.Where( + "repo_id = ? AND connection_id = ? AND (state != 'closed' OR commits = 0)", + repoId, connectionId, + ), + } +}