CircleCI Ideas

Show test results for prospective merge of a Github PR

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.

 

Source: https://discuss.circleci.com/t/show-test-results-for-prospective-merge-of-a-github-pr/1662

  • Avatar32.5fb70cce7410889e661286fd7f1897de Guest
  • Mar 5 2019
  • New
  • Attach files
  • Avatar40.8f183f721a2c86cd98fddbbe6dc46ec9
    Guest commented
    05 Mar 19:33

    The reason this feature is so important can be summed up with a single question we want to answer with every pull request:

     

    "If I merge this pull request onto master, will it work or will it break?"

     

    The answer a passing CircleCI build gives you today by just using the `checkout` command is "probably"

     

    Maybe a parameter to the "checkout" command? e.g.

     

    steps:
    - checkout:
    merge: true

     

    Either way, seems like answering that fundamental question in a simple yes or no fashion is foundational to the entire concept of CI

  • Avatar40.8f183f721a2c86cd98fddbbe6dc46ec9
    Guest commented
    05 Mar 19:37

    For reference, here is the description of the feature on Travis

     

    Rather than build the commits that have been pushed to the branch the pull request is from, we build the merge between the source branch and the upstream branch.

    https://docs.travis-ci.com/user/pull-requests/#how-pull-requests-are-built

  • Avatar40.8f183f721a2c86cd98fddbbe6dc46ec9
    Guest commented
    09 Mar 06:00

    This is a pretty major pain point.The problem is this:

    1. CI for master branch starts failing, for example because a dependency had a new release which causes a problem. (all CI's fail at this point)

    2. A fix is merged into master.

    3. All existing PRs that are updated, as well as all new PRs that are made from a not fully up-to-date master, still fail on CircleCI, but work on TravisCI, Appveyor, Azure Devops, etc.

     

    This is *very* common. CircleCI's default behavior is not useful; every other CI system builds the merge commit.

     

    Note that some people in the original thread suggested re-running the merge commit on every change to either master or the PR. That is not necessary in most cases, and would result in a massive increase in resource usage.

  • Avatar40.8f183f721a2c86cd98fddbbe6dc46ec9
  • Avatar40.8f183f721a2c86cd98fddbbe6dc46ec9
    Guest commented
    09 May 20:22

    For now, isn't this something we can implement ourselves using the features available CircleCI? Some people have also mentioned TravisCI offering that merge test but then they don't do the non-merged test. Doesn't that itself run into issues?

  • Avatar40.8f183f721a2c86cd98fddbbe6dc46ec9
    Guest commented
    17 May 20:01

    We have found a work around where we pull the `master` branch into the current branch before doing the build step. It works for now, but its quite brittle. This workaround won't work when the base branch isn't the `master` branch. It is a very basic feature I would expect in a CI/CD tool.

  • Avatar40.8f183f721a2c86cd98fddbbe6dc46ec9
    Guest commented
    01 Jul 16:47

    We implemented this feature ourselves but it adds on a bit of a burden to the code. And then the same practice has to be applied to every repository out there. And along with that, the functionality is not perfect. This issue is also coupled with the burden of not being able to trigger workflows properly (I believe the API for triggering workflows is flaky at best and im not sure it exists at the moment). 

    What we mainly wanted was to trigger builds for PRs as well as for the master branch + tags, so to cover that, we went with setting CircleCI such that the trigger would be via commit. But we're missing one case which is when you first make the PR, the build does not happen again when you have this setting.  

  • Avatar40.8f183f721a2c86cd98fddbbe6dc46ec9
    Guest commented
    13 Aug 08:41

    Is this as crucial as the comments describe? I'm fairly naive to CI/CD so apologies if I'm misunderstanding, but the merge commit would still be "the merge commit at the point of testing", which is still no guarantee of it breaking the target branch or not at the point of merge; you'd still have to retest the target to be certain.

    So this seems like extra security, but still the answer of "will it work" is still "probably"?

  • Avatar40.8f183f721a2c86cd98fddbbe6dc46ec9
    Guest commented
    02 Sep 03:10

    Yes, it is as crucial as the comments describe. It's the default on Travis, and never have I seen someone complaining about it. On the other hand, this enhancement have been asked for for at least 3 years.

    We do the following in our configuration:
    ```
    - run:
        name: Checkout merge commit
        command: |
            set -ex
            if [[ -n "${CIRCLE_PR_NUMBER}" ]]
            then
                FETCH_REFS="${FETCH_REFS}
                +refs/pull/${CIRCLE_PR_NUMBER}/merge:pr/${CIRCLE_PR_NUMBER}/merge"
                git fetch -u origin ${FETCH_REFS}
                git checkout "pr/${CIRCLE_PR_NUMBER}/merge"
            fi
    ```

    But I don't see any reason why it shouldn't be the default. Imagine merging a PR just to find out it goes red because someone added a test in between that fails with the new code. It's not a perfect solution (e.g. Travis does not trigger rebuild automatically on every merge, otherwise it'd overload the CI), but it's a lot better than the current default on CircleCI.

  • Avatar40.8f183f721a2c86cd98fddbbe6dc46ec9
    Guest commented
    13 Sep 13:00

    This is a very important feature