ejoffe / spr

Stacked Pull Requests on GitHub
MIT License
714 stars 65 forks source link

Fix status check not working for PRs without any status checks #405

Open Skipants opened 2 months ago

Skipants commented 2 months ago

The Github GraphQL response returns null for the StatusCheckRollup object if there are no status checks at all. In that case, this should be a pass.

From my understanding, spr gets the state of the PullRequest with a graphql query like the following:

gh api graphql -f query='
  query PullRequest($repo_owner: String!, $repo_name: String!, $pr_number: Int!) {
    viewer {
      login
    }
    repository(owner: $repo_owner, name: $repo_name) {
      pullRequest(number: $pr_number) {
        id
        number
        title
        body
        baseRefName
        headRefName
        mergeable
        reviewDecision
        repository {
          id
        }
        commits(first: 100) {
          nodes {
            commit {
              oid
              messageHeadline
              messageBody
              statusCheckRollup {
                state
              }
            }
          }
        }
      }
    }
  }' -f repo_owner='myrepo' -f repo_name='myrepo' -f pr_number=123

On the repository I am working on, this would give a response with a statusCheckRollup: null:

{
  "data": {
    "viewer": {
      "login": "Skipants"
    },
    "repository": {
      "pullRequest": {
        "id": "PR_fakeid123",
        "number": 123,
        "title": "My Cool PR",
        "body": "My Cool PR Stack\n\n---\n\n**Stack**:\n- #456\n- #123 ⬅\n\n\n⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing s
o may have unexpected results.*",
        "baseRefName": "master",
        "headRefName": "spr/master/abc123",
        "mergeable": "MERGEABLE",
        "reviewDecision": "APPROVED",
        "repository": {
          "id": "thisIsAFakeID="
        },
        "commits": {
          "nodes": [
            {
              "commit": {
                "oid": "123abc",
                "messageHeadline": "This is a cool commit",
                "messageBody": "Commit message\n\ncommit-id
:abc123",
                "statusCheckRollup": null
              }
            }
          ]
        }
      }
    }
  }
}

This prevented me from merging PRs in the stack as the status check would always fail. This fix makes null statusCheckRollups a pass instead.

I rebuilt this executable locally and was able to move my PRs along with no issue.

Skipants commented 2 months ago

I forgot to run tests and I have a feeling they might fail on this change. I changed this to a draft PR for the time-being.

Skipants commented 1 month ago

Tests pass

ejoffe commented 1 month ago

Sorry about the long delay. Thank you for the contribution.

Skipants commented 1 month ago

@ejoffe No problem at all. Huh looks like tests fail though, even though they passed locally. I'll take a look.

Skipants commented 4 weeks ago

Oops. I'm a little fresh to go and realized I only ran tests in spr/spr instead of doing go test ./... 🤦

Tests were failing because the fezzik type mocks returned no statusCheckRollup. So the default value for ChecksPass on these expected PR values were github.CheckStatusFail. It doesn't seem like we're actually testing the status checks in these tests so seems fine to just switch them all to github.CheckStatusPass.

@ejoffe If you'd like it done differently let me know. Otherwise I think you're good to merge this.