58

Suppose we have two branches A and B which have been forked from master.

Both branches A and B make some changes and implement some unit tests. They pass all current and new tests, then are merged back into master. For simplicity, there are no merge conflicts.

Is it guaranteed that the resulting code on master will also pass the unit tests?

The reason I ask the question, is I often see GitHub unit tests run automatically once a pull request is made. If they pass, then the code may be merged into master. However, I think master could still end up failing tests if two pull requests break each other? I would have thought a better solution would be:

  1. When a pull request is made, run the unit tests to catch anything egregious.
  2. Have conversations, code reviews etc...
  3. Once the pull request is ready to be merged, do a test merge into master, run the unit tests, if all succeeds, commit the merge.

So you never actually commit broken code into master.

3
  • 10
    This shows another advantage for only accepting fast-forwards to master (i.e. always rebase PRs before merging)
    – Pedro A
    Oct 6, 2020 at 18:52
  • 6
    @PedroA For smaller projects, I love enforcing fast forward merges. For bigger projects, you need to allow concurrent merges. The key is that you never deploy master if it's broken. You just have to fix it. Master getting broken by incompatible merges is just the nature of big problems. It happens, but it should still be rare if everything passes pre merge, and you aren't merging hours after tests pass. In general, the time it takes to ensure that master is never broken is more costly than just fixing master in the rare times in breaks from incompatible merges.
    – Cruncher
    Oct 6, 2020 at 19:27
  • This is why it is a good thing to do the merge in your own branch instead of on master. Then you can fix any breakage locally instead of halting the main build. Oct 7, 2020 at 8:29

11 Answers 11

156

No.

The simplest example I've seen is: branch A cleans unused imports in a file. Branch B adds code that actually uses some of the unused imports. Git merges automatically since the lines that were changed were not the same. Code can no longer compile and unit tests can not run.

8
  • 2
    Or in a dynamic language such as PHP it will compile, but then crash when the code that uses the import is executed and the auto-loader fails to find the referenced class without the import ('use') statement.
    – bdsl
    Oct 5, 2020 at 10:38
  • 3
    Which is precisely the reason why one should always retest each and every new commit after a rebase or merge: The resulting code might not even compile. And we don't want commits that won't even compile. Oct 5, 2020 at 12:05
  • 10
    the rule-of-thumb IMO for software is "it's not done till it's tested". so in my book you don't assume the merged code works... instead you always run the unit-tests to verify they pass and then take it from there. (the reason you don't assume it works is because sometimes merging two branches can create unintended behavior. you can get into a situation where you pass the unit-tests but the unintended behavior causes new issues and that is when you add new unit tests and fix the issues.) bottom line don't assume. Oct 5, 2020 at 13:38
  • 2
    @Glorfindel: I think that should remain "Code can no longer compile" (without the 's' - infinitive after modal verbs). Oct 6, 2020 at 10:27
  • 2
    I actually laughed out loud at this one. Good point! Unit tests cannot pass if they cannot run.
    – David K
    Oct 7, 2020 at 3:32
114

No. As a counter example, consider branch A adds a unit test that uses reflection to check for a misspelling in an enum. And branch B adds a misspelling. Both pass because a misspelling doesn’t fail a build, in A the test doesn’t fail because everything is spelled right, and in B there isn’t a test to check it. There won’t be any merge conflicts because the enum and its unit test will be in separate areas. But the test will fail once the merge is complete.

1
  • 40
    No need to resort to reflection (which particular language may or may not have). Even more general "A adds test that B doesn't pass" works.
    – Dan M.
    Oct 6, 2020 at 12:20
36

Here is an example which neither does require changes to the existing tests itself, nor reflection, nor a failing build, for not giving the wrong impression such cases can only happen under artificial circumstances.

Assume the codebase contains a private function f which is currently not called anywhere (maybe it was in the past, but noone has deleted it). Now in branch A a call to f is added internally in an existing function g during a refactoring. The unit tests for g show that everything works as intended, and g's behaviour hasn't changed.

At the same time, the devs working on B observed that with some modifications to the behaviour of f they could reuse it, too, and since f is not used elsewhere from their perspective, they thought the modifications to be safe. Maybe they also added some tests here which cover the new version of f, but that does not really matter. So as a result, in branch B, the behaviour of function f is changed.

To illustrate this with a real example, assume f originally to be a function like this:

 // normalize a given angle in degrees to the interval [0, 360[
 double NormalizeAngle(double angleInDegrees)
 {
 // ....
 }

and in branch B, the behaviour gets changed, making NormalizeAngle deliver values from the interval [-180, 180[.

So before the merge, no tests will fail. After the merge, the tests for function g will fail. Note if in B function f's signature would have been changed, both branches A and B will compile (assuming a compiled language environment), but after merge not even the build will be successful anymore. If the signature does not change, the compiler will not find this issue, only the unit tests.

12

Approaching this from a different angle, there's a simple process to ensure that the tests continue passing after merging both branches: a branch must pass CI after being applied to the current target branch before being merged. So when branch A merges, the goalpost for the other branches move to "main branch with A applied to it." To expedite this, most CI systems automatically trigger the CI pipeline for all the other pending branches when merging a branch.

Basically the only way to consistently break this "soft" guarantee would be for the tests to behave differently based on whether it's being run on the source or target branch.

4
  • "To expedite this, most CI systems automatically trigger the CI pipeline for all the other pending branches when merging a branch." This part interests my, as I thought it wasn't the case. Just to be clear, are you saying most re-trigger tests on other PRs each time master is updated?
    – kentrid
    Oct 5, 2020 at 11:42
  • 1
    I was under that impression, but I've been working with a rebase model for the last few years (a branch has to be rebased onto master before merging), so I might be projecting.
    – l0b0
    Oct 5, 2020 at 18:26
  • 5
    Unfortunately it's not that common @kentrid. Dan Luu reported many broken open-source builds. Graydon Hoare dubs this the "not rocket science" rule and had to build a tool to enforce it. Oct 6, 2020 at 1:35
  • @WesToleman Excellent articles, that pretty much sums up my view. Unfortunately integration tests are popular, so each test run inevitably ends up taking anywhere from a few minutes to several hours. This results in a huge amount of wasted time, disincentive to run all the tests all the time (nevermind in a loop), and just all round worse CI experience.
    – l0b0
    Oct 6, 2020 at 5:41
9

If two individual branches pass unit tests, once they're merged, is the result also guaranteed to pass unit tests?

Taking the question at face value, it's very simple to create an example where one branch only tests part of its codebase and has a bug in the untested part. Its unit tests pass, but there is a bug in the code.

Therefore, any test from the second branch that does test this (so far untested) piece of code might pass in its own branch (where it does not have the bug), but not when the bug from the first branch is merged into it.

The reason I asked the question, is I often see on GitHub unit tests run automatically on each pull request. If they pass, then the code is merged into master. Wouldn't it make more sense to unit test the resulting merge immediately before the actual merge is committed?

This, however, is a much more interesting question.

It makes sense to test the end result of a merge as well, but the way you're proposing to do it is not the best way.

First of all, your suggestion depends on the ability to auto-merge. Without auto-merging, your build process couldn't actually merge the two branches before testing its merged result.
When you can't auto-merge, you have to rely on what is currently already the suggested course of action: merging the master inside your feature branch before making your PR.

But let's say you can auto-merge. If you test the merged combined code, then the test failure can be caused by either an issue on the source branch (feature) or the target branch (master).
This means that your PR no longer reviews the feature branch itself, which makes it very hard to spot issues in specific feature branches.

The simplest example here is if a bug does make it onto the master branch, then every PR will fail, except a PR that solves the bug. While this may look appealing from the POV of someone who wants to keep the code clean, it's causing other issues. Every developer will now be troubleshooting their own PR build failure, unaware that there is a single bug causing all of the failures.

That's going to be quite inefficient in terms of having multiple developers independently locate the same bug. So let's say you try to counter that, by having developers confer before investigating a PR failure, so that not everyone tries to solve the same problem without coordinating.
But now, you've creating a point of distraction. Every time some developer makes a mistake in their own feature branch, they're needlessly requiring other developers to confirm that they themselves are not experiencing PR build failures. That, too, is a waste of time and effort.

Wouldn't it make more sense to unit test the resulting merge immediately before the actual merge is committed?

This does happen, but it's not the PR that does it. In most CI configurations I've worked in, the master branch goes through the build process whenever a PR gets merged into it. After all, a merge is just a kind of commit, and in a CI/CD environment you should be building your master after every commit made to it.

This breaks down the process in steps where the source of a step failure is easily identifiable.

  1. The feature branch PR tests the feature itself. If it fails, the feature branch is flawed. If it passes, the feature itself is considered finished.
  2. Post-merge, the master branch gets built. Any merge issues here will be reported as an issue in the master branch, not the feature branch, as it's not an issue with the feature itself, only its integration of the feature into the master.

In this answer I assumed you were working on a master branch instead of a separate dev branch.

The distinction between a master/dev branch is irrelevant as far as git merging goes; but this does highlight why the existence of a separate dev branch next to master has added value: dev acts as an integration branch which catches any issues with integrating a feature into the main codebase, before it makes its way into the master branch.

19
  • "But let's say you can auto-merge. If you test the merged combined code, then the test failure can be caused by either an issue on the source branch (feature) or the target branch (master)." Would this be the case anyway, if people are forking from master for the PR?
    – kentrid
    Oct 5, 2020 at 9:51
  • @kentrid: If more than one person develops this codebase, you cannot reasonably assume that the master has not changed inbetween you branching from it and trying to merge back into it. Even on my project where I'm the only contributor, I still have updates on the master branch during the development of a feature branch (e.g. bugfixes, or trivial changes unrelated to a larger feature).
    – Flater
    Oct 5, 2020 at 9:54
  • I helped to set up a system where tests were run on PRs (by simulating the merge). We thought this was a lot more sensible because we would like to know if master will stop working properly before we click the button that makes it stop working properly. It does take more test resources.
    – user253751
    Oct 5, 2020 at 10:48
  • @user253751: I'll be honest here and admit that I initially thought OP's suggestion was a really good addition. I'm still not convinced that it's bad, but I am wondering exactly how much value it adds (how often such an issue occurs, the impact of it occurring, how long it takes to normally fix, ...) compared to the added cost of doing a second build process.
    – Flater
    Oct 5, 2020 at 10:52
  • 1
    @Flater I think my question boils down to testing before committing a merge to master, or testing after? At the moment, it seems most do it after. However, it seems to make more sense to do it before, as if the test fails, you haven't then committed breaking changes to master?
    – kentrid
    Oct 5, 2020 at 11:57
3

No.

The solution to your problem is to take advantage of the fact that git is distributed and run your automated tests against the product of the desired merge locally (ideally on a CI runner) before pushing that merge commit to the shared repository (ideally performed by that CI runner).

Why this isn't the default paradigm for this sort of thing is completely beyond my comprehension.

4
  • 1
    You can't trust tests run locally. Sure run them, it will catch a lot, but it has to be tested by the build farm in a clean environment before other devs can pull down your code.
    – corsiKa
    Oct 7, 2020 at 8:34
  • 1
    @corsiKa 'local' to the machine that pulled the code. That doesn't necessarily mean your workstation. Oct 7, 2020 at 15:39
  • 1
    Ah. Beautiful. Yes, everything you're saying makes perfect sense now. Absolutely, your CI engine pulls and builds your change and tests it before allowing the commit to be pushed to other devs. You can set this up with reliable open source for any programming language and almost any flavor of code repo in only a few hours with a couple youtube tutorials, even though I'd expect any competent developer to be able to be able to set it up even without those. It baffles me how many companies ignore this low hanging ROI.
    – corsiKa
    Oct 7, 2020 at 18:19
  • 1
    @corsiKa Yeah honestly I really don't get why gitlab/github don't just do this by default out of the box. I definitely understand why the more typical behavior is an option, but this should be an extremely common case for any "protected" branches. Oct 7, 2020 at 18:25
2

As the other answer stated, no, passing tests on 2 non-conflicting branches are not enought to say there won't be failures after merging them both. You have plenty of examples.

Let me focus on the second part of the question, the proposed flow, how it may fail and how it might be approached:

  1. When a pull request is made, run the unit tests to catch anything egregious.
  2. Have conversations, code reviews etc...
  3. Once the pull request is ready to be merged, do a test merge into master, run the unit tests, if all succeeds, commit the merge.

This is a nice and sound process and perhaps a great one for a small project. It really ensures no failures in master, and it's quite simple. There is one big issue with it: it doesn't scale. It doesn't scale at all.

With this process you drafted you must serialize the commits, and this gets very costly very fast when the project grows.

For instance, if you have 3 pull requests, you need to test-merge the first one, run all the tests, then update master. Only then you can start testing the test-merge of the second branch, and only after it's in, you can start running the proper tests for the last branch.

This means that if your test suite takes 3 minutes, you can make at most 1 commit in every 3 minutes. That's inconvenient, but feasible. However, if your test suite takes 10 minutes, you are limited to 6 merges per hour at best, 48 merges per work day. A team of 20 people working with such a constraint would spend half their time babysitting the pull requests, and you could end up with a typo fix waiting half a day to be merged.

Worse yet, if your test suit take several hours and you have many thousands of developers working on a single monorepo, producing tens or hundreds of commits per minute... well, you see the problem. Even running the continuous integration after every merge makes little sense in this case.

What is more scalable?

Focus on the continuous integration and quick rollbacks instead of preventing all bad merges from happening. Also track the test failures, so that you can guess with high confidence whether a test failure is caused by the branch, or by the broken master (a smart enough testing tool will annotate them as "already failing", and you may vote to allow merging with this kind of failures). You don't even need to run the integration after each merge (it's the simplest starting point, but doesn't scale to the really huge projects), it may be every few minutes or every hour, depending on how much resources you want to throw at it.

If you detect no failures, everything is fine. If you detect a failure, you can run a binary search over the changes to determine which one caused the specific test to fail - this is relatively cheap, because usually you won't have half the tests fail, just a handful of them.

On top of that, leverage your build system in order to determine the set of builds and tests that actually may be affected by each change, and limit the required test suite to these. Also, as part of CI run these selected tests immediately after the merge, in order to detect the issues as quickly as feasible (separately from the full test suit running once in a while). The determination doesn't have to be watertight - in case you miss a test that is actually affected, the "big" integration testing will still catch it, just a while later, but most of the time you'll get the feedback quite fast.

The flow I described is loosely based on what Google does internally, I assume it's similar to what other big companies do as well. It's worth pointing out that no popular VCSs supports the monorepos as big as theirs, at least not the vanilla version.

In case of Google, the VCS is Perforce based, and it has much stricter rule for conflicts - any other change in the same file is a conflict, no matter how close or far apart are the changes. This eliminates quite a bunch of pitfalls, like those with removed imports - the change would have to be updated and rebuilt, and the issue would show up, similarly to the process you proposed. So this is one more counter-measure - just tighten up the rules for what can be merged to the master. While requiring "only fast-forward changes with passing tests" (i.e. your proposed rule) is unfeasible at scale, "only changes that are fast-forward with regard to affected files" can scale up relatively well.

6
  • Having worked for a company that made CI software in the past, I can say your point about limiting the builds is indeed a problem small to medium software shops face. However, large companies get around it with a build farm - a whole collection of servers dedicated to building software and running those tests. But you DO need to run the integration after every merge. Every time. No exceptions. The sooner you get feedback to your developer the cheaper it will be to fix it.
    – corsiKa
    Oct 6, 2020 at 15:20
  • @corsiKa If the repository consisting of a single project, you are probably right. However, for a monorepo consisting of hundreds of independent projects it simply doesn't make sense (and also no matter how big the build farm is, it is still going to take hours). I mean, you will actually get faster feedback by making the full run every 10, 100, or 1000 commits, finding the failing tests and only then re-running just these tests in a binsearch fashion: this way you save tons of capacity, allowing the whole system to work faster.
    – Frax
    Oct 6, 2020 at 15:53
  • This is slightly different for some heavy integration tests, that are run only for changes touching specific directories/files, but likely on every such a change indeed.
    – Frax
    Oct 6, 2020 at 15:55
  • I will grant you that huge companies can make stupid decisions like putting hundreds of projects in a single repo. I've never seen it, but sure it could happen. Assuming that at least one person has ever seen a build farm before, though, I doubt it would last too long. But in years of consulting and practice I've never seen what you describe. You build everything any time the code is touched in a project.
    – corsiKa
    Oct 7, 2020 at 8:32
  • @corsiKa Wikipedia list a few examples of large companies using monorepos, and a rather comprehensive list of advantages. You are free to assume that they are making a stupid decision. However, I assure you that they are not stupid enough to run every sigle test every time someone fixes a typo is a comment. Well, I can speak about Google at least, because I have seen it: a large monorepo, not every test is run after each change, tests can run for hours, but the constantly running CI finds all the issues relatively quickly.
    – Frax
    Oct 7, 2020 at 15:11
1

Interesting question, I gave it some thought and came up with the following situation in which 2 branches which are independently correct, result in a merge which breaks the build.

Suppose in the 2 branches a function/procedure with same name and signature is added to a class. However this is done in different locations or even different files. The resulting merge will result in a class with 2 identical functions/procedures and will therefore give a compile-error.

1

Another problem scenario is that the original program performed a safety check in both a private function, and in its calling code. Branch A removes the check from the function, and branch B removes it from the calling code. Both branches will perform the safety check once, would for most purposes be an improvement over calling it twice, but merging the changes will cause the safety check to be omitted altogether. A test for safety-check behavior would thus pass on both branches, and the merged code would appear to work fine if the checked condition never arises, but code would no longer be protected against the condition that was supposed to be guarded by the safety checks.

1

Let's look on how to prevent the problem. As mentioned in the question, CI (typically) reruns after every commit to the default branch, including merges. Some of the answers already explain how things can break.

Some other answers suggest a dev branch and fast-forward the main branch only when the CI is stable on dev. But this would require additional manual intervention and can become a hassle on a big project.

And then there is the Bors-ng tool:

Bors is a GitHub bot that prevents merge skew / semantic merge conflicts, so when a developer checks out the main branch, they can expect all of the tests to pass out-of-the-box.

Basically, accepted pull requests are merged with the main branch in a temporary branch. When CI passes, the main branch gets Fast Forwarded to the temporary branch. It is quite feature complete and supports reviewers, batching of builds, queuing (when it gets really busy) and (I believe) most CI providers.

Disclaimer: I have no affiliation with this product, just a happy user.

1

Of course there is no guarantee. The examples are legion.

But.

It is not unreasonable to assume that unrelated, isolated changes are unlikely to break anything. Performance improvements in a backend algorithm are unlikely to change the database interface. This is the same assumption that's underlying the paradigm of unreserved checkouts/parallel development of which git is a prime example: Hopefully the team communicates well and organizes work packages in a fashion so that they do not conflict, or, if that is impossible, organizes conflicting work so that the arising problems are predictable and handled proactively. (Then, ideally, we know that a naive merge is broken.)

1
  • The problem can be mitigated some of the time if team members can pull each other's branches, then do a merge locally to run unit tests on the merged result. That way, they can be sure that the current state of the repository won't break any tests when the pull requests are merged. Of course, there's still no guarantee that future commits or branches won't break any tests, but all team members can collectively agree to do unit tests locally on merged results always before pushing a new branch or opening a new PR. (Nice answer btw.)
    – ADTC
    Mar 3, 2022 at 7:53

Not the answer you're looking for? Browse other questions tagged or ask your own question.