Show test results for prospective merge of a Github PR


#1

Github PRs show if the PR branch has conflicts with the base branch.

CircleCI shows test results on the PR branch.

What is missing is the ability to see whether test results fail when the PR branch is merged into the base branch.

Admittedly, it’s not everyday that PRs break tests of other PRs, but it does happen - for example when there’s a change in UI text:

  1. A feature developer issues PR A testing for some specific text in the UI: “The status is FOO”. PR A contains a view test checking for the existence of “The status is FOO”.
  2. A another developer issues PR B that implements a product decision to change all uses of “FOO” to “BAR”. Github says branch is clear to merge
  3. PR B is merged (Github says there are no conflicts; CircleCI says tests pass)
  4. PR A is merged, without problem (Github says there are no conflicts; CircleCI says tests pass)
  5. The base branch now has a test which fails because the application produces a view with the text “The status is BAR”, but the test in PR A expects “The status is FOO”.

Fail.

All of the ways of dealing with this currently seem bad:

  1. Constantly merge into the PR branch, making a mess of the commit history
  2. Rebase the PR branch onto the base and force push onto Github (a no no for obvious reasons)
  3. Catch the test failures later in the base branch (inconsistent with gitflow and other branching strategies)

A feature would be nice, but a work around would be great too.


PRs should also build the temporary merge with target branch
Pull requests not triggering build
#2

Another possibility is that CircleCI could be configured to rebase the PR on target branch latest before running tests and the rest of the flow.

Fail in cases of merge conflict or tests failing now that the change is together with latest code. I’ve seen this done to good effect in some other CI systems.


#3

Travis does this and it’s very nice.


#4

For reference, Travis CI already has this feature: https://docs.travis-ci.com/user/pull-requests

It’s simply a matter of building merge_commit_sha rather than sha: https://developer.github.com/v3/pulls/#get-a-single-pull-request

It would be great if this was the default for CircleCI or at least an option to switch.


#5

This would be a great feature! We just ran into this issue on TSLint recently. A PR was based off an out-of-date commit, and its tests were passing. However, once it was merged, tests were broken. Running tests on the result of the merge (before it’s actually merged) would have prevented this issue.


#6

The issue with this feature is that every commit on the destination branch will require tests to be rerun on the remaining PRs to that branch. There are a couple of solutions: automatically rerun the tests (which might be a good thing from CircleCI’s perspective since it would create more demand for containers) OR show that the test results passed previously but are now out of date and let the user manually trigger another test run.


#7

This should be totally doable honestly. GitHub provides a way to checkout a merge ref for a PR instead of the head ref (what is done now). In other words, refs/pull/<PR#>/merge where <PR#> is the pull request number will give you the result of the merge.

The one catch is one must check to see if the merge will have conflicts first. As GitHub currently leaves a merge ref that may be outdated if a conflict occurs later. See issue ( https://github.com/isaacs/github/issues/757 ) for a description of this behavior. However, this can easily be checked with the GitHub API.


#8

So this is a nearly working solution. The reason it is nearly is that it doesn’t detect merge conflicts well and so can provide false results in some cases. More about this below.

checkout:
  post:
    - |
        if [[ -n "${CIRCLE_PR_NUMBER}" ]]
        then
            # Update PR refs for testing.
            FETCH_REFS="${FETCH_REFS} +refs/pull/${CIRCLE_PR_NUMBER}/head:pr/${CIRCLE_PR_NUMBER}/head"
            FETCH_REFS="${FETCH_REFS} +refs/pull/${CIRCLE_PR_NUMBER}/merge:pr/${CIRCLE_PR_NUMBER}/merge"

            # Retrieve the refs
            git fetch -u origin ${FETCH_REFS}

            # Checkout PR merge ref.
            git checkout -qf "pr/${CIRCLE_PR_NUMBER}/merge"

            # Test for *some* merge conflicts.
            git branch --merged | grep "pr/${CIRCLE_PR_NUMBER}/head" > /dev/null
        fi

Now merge refs are not always deleted like they should be when there is a merge conflict (my previous post and xrefs discuss this in more detail). As a result if we want to fail a build with a merge conflict immediately (either to notify of the merge conflict or to save build resources), we need to make a couple of tweaks.

To do this, we need to know the base branch of the PR. This should be easy for CircleCI to give us and I have asked for it in this post. Though have yet to hear back. Assuming we have the base branch in an environment variable and it is called CIRCLE_PR_BASE_BRANCH, we can do the following.

checkout:
  post:
    - |
        if [[ -n "${CIRCLE_PR_NUMBER}" ]]
        then
            # Update PR refs for testing.
            FETCH_REFS="+${CIRCLE_PR_BASE_BRANCH}:${CIRCLE_PR_BASE_BRANCH}"
            FETCH_REFS="${FETCH_REFS} +refs/pull/${CIRCLE_PR_NUMBER}/head:pr/${CIRCLE_PR_NUMBER}/head"
            FETCH_REFS="${FETCH_REFS} +refs/pull/${CIRCLE_PR_NUMBER}/merge:pr/${CIRCLE_PR_NUMBER}/merge"

            # Retrieve the refs
            git fetch -u origin ${FETCH_REFS}

            # Checkout PR merge ref.
            git checkout -qf "pr/${CIRCLE_PR_NUMBER}/merge"

            # Test for merge conflicts.
            git branch --merged | grep ${CIRCLE_PR_BASE_BRANCH} > /dev/null
            git branch --merged | grep "pr/${CIRCLE_PR_NUMBER}/head" > /dev/null
        fi

The first key line added fetches the base branch and updates ours to match. As CircleCI often caches this, we want to make sure that we have an up-to-date version. The second key line checks to see if the base branch is immediately reachable from the PR merge ref. If it is not, we have a merge conflict and we error out.

Hope this helps some people. :smile:


Add environment varible CIRCLE_PR_BASE_BRANCH
#9

Adding my vote. Having used TravisCI on other projects, building the merge solves the problem about 95% of the time. There are still those rare cases where conflicting PRs get built and merged out of order, but most of the conflicts do get caught and it can avoid a lot of “emergency” fixes in master.


#10

Hello,

Thank you for your report. So that I can better understand your situation, are you suggesting that we run two builds for each PR including the commit and one for the merge-commit?


#11

I think the idea is that a build is done each time the merge commit would change. Ie, both when the PR is issued and any time the branch that the PR wants to merge into is affected. So a side effect of this would be that if you had several PRs waiting to be merged into base branch, each merge triggers a rebuild of all the remaining PRs, since the prospective merge would be affected for all of them.


#12

If I’m understanding correctly, the workflow you’re looking for isn’t currently supported.

Please understand that trying to accommodate every possible workflow is an impossible goal.

We do appreciate and value your feedback, and will consider this as we build out new features that enable you to choose your own workflow that best suits your needs.

Best,
Zak


#13

I am also interested in this feature. For me, assuming the branch is mergable, I don’t need to run the tests twice. Once on the post merged code would be enough.


#14

The solutions above are more complex than they need to be. Here is a one line solution you can add to your circle.yml to always test your PRs merged with master (or whatever branch they were opened against):

checkout:
  post:
    git pull --ff-only origin "refs/pull/${CI_PULL_REQUEST//*pull\//}/merge"

This will try to merge the GitHub ref, but it will fail if it cannot be fast forwarded (which will happen if the Github merge ref is out of date, i.e. your PR does not merge cleanly with master).

Since we have a large team, I wrapped it up in a script with some helpful error text:


#15

Please understand that trying to accommodate every possible workflow is an impossible goal.

Does any other workflow make sense?

The current workflow is sub-optimal. As a repository maintainer evaluating a pull request, the highest value question I want my CI to answer is “will this work if I press the merge button?” not “does it work on the source branch?”

This is why it’s the default on travis https://docs.travis-ci.com/user/pull-requests#How-Pull-Requests-are-Tested


#17

It’s a bit of a heavyweight solution, I can understand that, but it seems like it’s really valuable to a lot of people. From CircleCI’s perspective this should be a win - if people want the feature and enable it, it’s going to push them to purchasing more containers.


#18

That would be ideal and I think solve the issue that people are looking to have addressed here. Travis CI, for instance, accomplishes this by doing the following.

First allowing a build to proceed under a user’s account (i.e. their fork of the repo) with the branch they pushed to. This basically would work as is now with no real changes.

Second allowing a build to proceed on the main repo with the merge commit from the PR. Right now it seems this is not possible as the build will run under the user’s CircleCI project if they created one. So this would need to explicitly allow a second build for running on the main repo independent of the user’s build. My suggestion above would probably help achieve this second objective. One would also need to listen for opened, reopened, and possibly closed (to cancel a running build) PullRequestEvents on the webhook. Plus there would be some updates to the CircleCI status of the PR to accommodate this.


#19

Thanks for the share. It is a nice idea and I do like the simplicity.

However there is one caveat. The merge ref is not updated every time the base branch is updated. So if other PRs get merged and one simply restarts CircleCI, the merge ref will not include these recently merged PRs. This will make it appear like there is no conflict, but the latest state of the base branch is not tested and these PRs are missed. In cases like these, I would prefer an outright failure as early in the build as possible.

Now one can easily solve this issue by closing and reopening a PR or pushing a dummy commit to get the merge ref updated. So maybe this doesn’t matter much to most people. Still it is good to be aware of the limitations of any one approach.

Note: It is always possible GitHub has fixed this behavior. Had emailed them about it last year. So if things have changed, that would be good to know as well.


#20

The proposed workaround of doing a merge in the checkout: post: key has a serious problem: if I change the .circleci/config.yml on master, PRs will still be tested against the configuration that lives on the branch. So I might see a green circleCI status but actually the master will be broken, because the PR is not running a newly added test.


#22

It seems like the current way of building commits instead of merge commits for PRs means that if you have two PRs open from the same head branch but that target different base branches, they will share the same build. That is fundamentally wrong. If I need to merge a hotfix into both the develop and production branch, I need to verify that that change will not break anything on either of the two base branches. The diff for each PR can look very different, yet they share the same build. Instead, we need one build for each PR.

Using something like commitlint-cli to lint the commit range of the PR to follow a commit message convention becomes impossible because the build actually belongs to two PRs with different bases, so there are acutally two commit ranges. There is no clear way to figure out the commit range to lint. If you lint both and validation fails, both PRs will fail, even though only one of them contains the violating commit message.

This is a really important aspect for the correctness and trustworthiness of a CI system. If the user wants to prevent triggering both a PR build and a branch build, it’s always possible to only build master and PRs (after all, that is what you should test. Prefix it with WIP if you want and use something like wipbot to prevent merges). AppVeyor also has an option to skip branch builds that have an open PR.