diff --git a/common/reviews.go b/common/reviews.go index 5da3c25..80737da 100644 --- a/common/reviews.go +++ b/common/reviews.go @@ -17,15 +17,16 @@ type PRReviews struct { } func FetchGiteaReviews(rf GiteaReviewTimelineFetcher, org, repo string, no int64) (*PRReviews, error) { + rawReviews, err := rf.GetPullRequestReviews(org, repo, no) + if err != nil { + return nil, err + } + timeline, err := rf.GetTimeline(org, repo, no) if err != nil { return nil, err } - rawReviews, err := rf.GetPullRequestReviews(org, repo, no) - if err != nil { - return nil, err - } reviews := make([]*models.PullReview, 0, 10) needNewReviews := []string{} @@ -187,8 +188,6 @@ func (r *PRReviews) HasPendingReviewBy(reviewer string) bool { switch r.State { case ReviewStateRequestReview, ReviewStatePending: return true - default: - return false } } } @@ -201,18 +200,20 @@ func (r *PRReviews) IsReviewedBy(reviewer string) bool { return false } - for _, r := range r.Reviews { - if r.User.UserName == reviewer && !r.Stale { - switch r.State { + res := false + + for _, i := range r.Reviews { + if i.User.UserName == reviewer && !i.Stale { + switch i.State { case ReviewStateApproved, ReviewStateRequestChanges: - return true - default: + res = true + case ReviewStateRequestReview, ReviewStatePending: return false } } } - return false + return res } func (r *PRReviews) IsReviewedByOneOf(reviewers ...string) bool { diff --git a/integration/test-plan.md b/integration/test-plan.md index c9ae045..0a708de 100644 --- a/integration/test-plan.md +++ b/integration/test-plan.md @@ -65,11 +65,11 @@ The testing will be conducted in a dedicated test environment that mimics the pr | **TC-REVIEW-001** | P | **Add mandatory reviewers** | 1. Create a new PackageGit PR. | 1. All mandatory reviewers are added to both the PackageGit and ProjectGit PRs. | High | | **TC-REVIEW-002** | - | **Add advisory reviewers** | 1. Create a new PackageGit PR with advisory reviewers defined in the configuration. | 1. Advisory reviewers are added to the PR, but their approval is not required for merging. | Medium | | **TC-REVIEW-003** | - | **Re-add reviewers** | 1. Push a new commit to a PackageGit PR after it has been approved. | 1. The original reviewers are re-added to the PR. | Medium | -| **TC-REVIEW-004** | x | **Package PR created by a maintainer** | 1. Create a PackageGit PR from the account of a package maintainer. | 1. No review is requested from other package maintainers. | High | +| **TC-REVIEW-004** | P | **Package PR created by a maintainer** | 1. Create a PackageGit PR from the account of a package maintainer. | 1. No review is requested from other package maintainers. | High | | **TC-REVIEW-005** | P | **Package PR created by an external user (approve)** | 1. Create a PackageGit PR from the account of a user who is not a package maintainer.
2. One of the package maintainers approves the PR. | 1. All package maintainers are added as reviewers.
2. Once one maintainer approves the PR, the other maintainers are removed as reviewers. | High | | **TC-REVIEW-006** | P | **Package PR created by an external user (reject)** | 1. Create a PackageGit PR from the account of a user who is not a package maintainer.
2. One of the package maintainers rejects the PR. | 1. All package maintainers are added as reviewers.
2. Once one maintainer rejects the PR, the other maintainers are removed as reviewers. | High | | **TC-REVIEW-007** | P | **Package PR created by a maintainer with ReviewRequired=true** | 1. Set `ReviewRequired = true` in `workflow.config`.
2. Create a PackageGit PR from the account of a package maintainer. | 1. A review is requested from other package maintainers if available. | High | -| **TC-MERGE-001** | x | **Automatic Merge** | 1. Create a PackageGit PR.
2. Ensure all mandatory reviews are completed on both project and package PRs. | 1. The PR is automatically merged. | High | +| **TC-MERGE-001** | P | **Automatic Merge** | 1. Create a PackageGit PR.
2. Ensure all mandatory reviews are completed on both project and package PRs. | 1. The PR is automatically merged. | High | | **TC-MERGE-002** | - | **ManualMergeOnly with Package Maintainer** | 1. Create a PackageGit PR with `ManualMergeOnly` set to `true`.
2. Ensure all mandatory reviews are completed on both project and package PRs.
3. Comment "merge ok" on the package PR from the account of a package maintainer for that package. | 1. The PR is merged. | High | | **TC-MERGE-003** | - | **ManualMergeOnly with unauthorized user** | 1. Create a PackageGit PR with `ManualMergeOnly` set to `true`.
2. Ensure all mandatory reviews are completed on both project and package PRs.
3. Comment "merge ok" on the package PR from the account of a user who is not a maintainer for that package. | 1. The PR is not merged. | High | | **TC-MERGE-004** | - | **ManualMergeOnly with multiple packages** | 1. Create a ProjectGit PR that references multiple PackageGit PRs with `ManualMergeOnly` set to `true`.
2. Ensure all mandatory reviews are completed on both project and package PRs.
3. Comment "merge ok" on each package PR from the account of a package maintainer. | 1. The PR is merged only after "merge ok" is commented on all associated PackageGit PRs. | High | diff --git a/integration/tests/workflow_pr_merge_test.py b/integration/tests/workflow_pr_merge_test.py index 2d09c7e..d297ed8 100644 --- a/integration/tests/workflow_pr_merge_test.py +++ b/integration/tests/workflow_pr_merge_test.py @@ -5,7 +5,6 @@ from pathlib import Path from tests.lib.common_test_utils import GiteaAPIClient @pytest.mark.t001 -@pytest.mark.xfail(reason="The bot sometimes re-request reviews despite having all the approvals") def test_001_automerge(automerge_env, test_user_client): """ Test scenario: diff --git a/integration/tests/workflow_pr_review_test.py b/integration/tests/workflow_pr_review_test.py index a6f2d45..788dbfa 100644 --- a/integration/tests/workflow_pr_review_test.py +++ b/integration/tests/workflow_pr_review_test.py @@ -43,7 +43,6 @@ index 0000000..e69de29 @pytest.mark.t004 -@pytest.mark.xfail(reason="the bot sometimes re-requests review from autogits_obs_staging_bot despite having the approval") def test_004_maintainer(maintainer_env, ownerA_client): """ Test scenario: @@ -152,7 +151,6 @@ index 0000000..e69de29 @pytest.mark.t005 -# @pytest.mark.xfail(reason="TBD troubleshoot") def test_005_any_maintainer_approval_sufficient(maintainer_env, ownerA_client, ownerBB_client): """ Test scenario: