42

From the technical point of view it is possible to add some pre/post push hooks which will run unit tests before allowing some specific commit to be merged to remote default branch.

My question is - is it better to keep unit tests in build pipeline (thus, introducing broken commits to repo) or it's better just not to allow "bad" commits to happen.

I do realize that I'm not limited with this two options. For instance, I can allow all commits to branched and tests before pushing merge commit to repo. But if you have to choose between exactly this two solutions, which one you'll choose and for what exactly reasons?

35

No, it's not, for two reasons:

Speed

Commits should be fast. A commit which takes 500 ms., for example, is too slow and will encourage developers to commit more sparingly. Given that on any project larger than a Hello World, you'll have dozens or hundreds of tests, it will take too much time to run them during pre-commit.

Of course, things get worse for larger projects with thousands of tests which run for minutes on a distributed architecture, or weeks or months on a single machine.

The worst part is that there is not much you can do to make it faster. Small Python projects which have, say, hundred unit tests, take at least a second to run on an average server, but often much longer. For a C# application, it will average four-five seconds, because of the compile time.

From that point, you can either pay extra $10 000 for a better server which will reduce the time, but not by much, or run tests on multiple servers, which will only slow things down.

Both pay well when you have thousands of tests (as well as functional, system and integration tests), allowing to run them in a matter of minutes instead of weeks, but this won't help you for small scale projects.

What you can do, instead, is to:

  • Encourage developers to run tests strongly related to the code they modified locally before doing a commit. They possibly can't run thousands of unit tests, but they can run five-ten of them.

    Make sure that finding relevant tests and running them is actually easy (and fast). Visual Studio, for example, is able to detect which tests may be affected by changes done since the last run. Other IDEs/platforms/languages/frameworks may have similar functionality.

  • Keep the commit as fast as possible. Enforcing style rules is OK, because often, it's the only place to do it, and because such checks are often amazingly fast. Doing static analysis is OK as soon as it stays fast, which is rarely the case. Running unit tests is not OK.

  • Run unit tests on your Continuous Integration server.

  • Make sure developers are informed automatically when they broke the build (or when unit tests failed, which is practically the same thing if you consider a compiler as a tool which checks some of the possible mistakes you can introduce into your code).

    For example, going to a web page to check the last builds is not a solution. They should be informed automatically. Showing a popup or sending an SMS are two examples of how they may be informed.

  • Make sure developers understand that breaking the build (or failing regression tests) is not OK, and that as soon as it happens, their top priority is to fix it. It doesn't matter whether they are working on a high-priority feature that their boss asked to ship for tomorrow: they failed the build, they should fix it.

Security

The server which hosts the repository shouldn't run custom code, such as unit tests, especially for security reasons. Those reasons were already explained in CI runner on same server of GitLab?

If, on the other hand, your idea is to launch a process on the build server from the pre-commit hook, then it will slow down even more the commits.

  • 4
    I agree, this is what a build server is for. Your source control is for managing source code, not ensuring your application works. – Matthew Oct 24 '14 at 0:09
  • 4
    In extreme cases, retaliation might be a necessary notification tool when breaking the build. – user40980 Oct 24 '14 at 0:31
  • 37
    Half a second is too slow? That's a drop in the bucket compared to giving one final look at what's being commited and then thinking of and typing an appropriate commit comment. – Doval Oct 24 '14 at 0:48
  • 5
    @Doval: the difference is that when you give one final look or think about the comment, you are proactive, and so, you are not waiting. It's not the time you spend before typing the last character in your IDE and the time when you can start typing again once the commit is finished that matters, but how much do you wait. That's why compilers should be fast; it doesn't matter that you spend much more time reading and writing code, because when you do that, you're not waiting, whereas when the code is compiling, you're tempted to switch to a different activity. – Arseni Mourzenko Oct 24 '14 at 3:15
  • 10
    @Thomas: it's not about distraction, but about annoyance. In the same way, 100 ms. "has a measurable impact" on how persons are using a website. Same pattern here: 100 ms. is nothing compared to the time you spend watching a YouTube video or starting your PC: consciously, you won't notice the difference between 600 ms. and 700 ms. delay. But unconsciously, it has an influence on your behavior. In the same way, slightly slower commits discourage you from committing early and frequently. – Arseni Mourzenko Oct 24 '14 at 6:49
40

Let me be the one to disagree with my fellow answerers.

This is known as Gated Check-ins in the TFS world, and I expect elsewhere. When you attempt to check-in to a branch with the gated check-in, the shelveset is sent off to the server, which makes sure your changes build and the specified (read: all) unit tests pass. If they don't, it notifies you that you're a bad monkey who broke the build. If they do, the changes go into source control (yay!).

In my experience, gated check-ins are one of the most important processes for successful unit testing - and by extension, software quality.

Why?

  • Because gated check-ins force people to fix broken tests. As soon as broken tests become something people can do rather than must do, they become de-prioritized by lazy engineers and/or pushy business people.
    • The longer a test is broken, the harder (and costlier) it is to fix.
  • Because as soon as people should run the tests rather than must run the tests, running the tests are circumvented by lazy/forgetful engineers and/or pushy business people.
  • Because as soon as unit tests impact your commit time, people really start caring about making their tests unit tests. Speed matters. Reproducability matters. Reliability matters. Isolation matters.

And of course, there's the benefit you brought up originally - when you have gated check-ins and a solid suite of tests, every single changeset is "stable". You save all that overhead (and potential for error) of "when was the last good build?" - all builds are good enough to develop against.

Yes, it takes time to build and run the tests. In my experience 5-10 minutes for a good sized C# app and ~5k unit tests. And I don't care about that. Yes, people should check-in frequently. But they should also update their tasks frequently, or check their email, or get some coffee or dozens of other "not working on code" things that make up a software engineer's job to occupy that time. Checking out bad code is far more costly than 5-10 minutes.

  • 3
    +1 I want to add, that many open source projects have a clear distinction between contribution and committing. The reasons for it are very similar to why gated check-ins exist. – JensG Oct 24 '14 at 19:48
  • One significant problem with gated check-in is it inhibits collaborative problem solving of difficult code. This is even more significant with distributed teams. – CuriousRabbit Nov 1 '14 at 14:35
  • 2
    @CuriousRabbit - how do you figure? People rarely commit collaboratively even if they work collaboratively. Otherwise shelvesets or undated task branches work good for that while not hindering the rest of the team. – Telastyn Nov 1 '14 at 16:05
  • @Telastyn– Shelvesets is a new term for me. I gather that this is an MS VS option, which is no option for a vast number of projects. In the field of mobile app development, VS is a non-player. – CuriousRabbit Nov 2 '14 at 20:10
  • 3
    @CuriousRabbit - really? A team distributed over 17 hours cares more about waiting 10 minutes for a commit than the possibility of a broken build while the offending party is asleep? That seems... Less than optimal. – Telastyn Nov 2 '14 at 20:11
40

Commits should run fast. When I commit some code I want it to get pushed to the server. I don't want to wait a few minutes while it runs a battery of tests. I am responsible for what I push to the server and don't need anyone babysitting me with commit hooks.

That said, once it gets to the server, it should be analyzed, unit tested, and built immediately (or within a short time frame). This would alert me to the fact that unit tests are broken, or it didn't compile, or I made a mess shown by the static analysis tools available. The faster this is done (the build and analysis), the faster my feedback and the faster I am able to fix it (the thoughts haven't completely swapped out of my brain).

So no, don't put tests and such in commit hooks on the client. If you must, put them on the server in a post commit (because you don't have a CI server) or on the CI build server and alert me appropriately to issues with the code. But do not block the commit from happening in the first place.

I should also point out that with some interpretations of Test Driven Development, one should check in a unit test that breaks first. This demonstrates and documents the bug is present. Then a later checkin would be the code that fixes the unit test. Preventing any checkins until the unit tests pass would reduce the effective value of checking in a unit test that fails to document the issue.

Related: Should I have unit tests for known defects? and What is the value of checking in failing unit tests?

  • 2
    Commits should run fast. what are the benefits of this? I'm curious because we currently use gated checkins. Usually my checkins are a cumulation of an hour or so of work, so a 5 minute wait isnt a big deal. In fact I've found that its normally the times when I am in a rush that the validation build is most useful for catching silly mistakes (as a result of rushing) – Justin Nov 5 '14 at 9:07
  • 1
    @Justin A five minute wait is a five minute wait no matter where it is. One shouldn't need to break out nerf swords every time you do a commit. And it is not uncommon for me to break an hour or so of work into multiple commits that are each conceptual units of each other - "commit the rest service code" followed by "commit the code for the served client page" as two separate commits (not to mention the css tweak as another commit). If each of these took 5 minutes to run, 1/6th of my time is spent waiting for tests. This would lead to larger rollup commits which are harder to track bugs in. – user40980 Oct 27 '15 at 13:18
  • 5 min wait to run unit tests? I feel like the projects you guys are working on should be broken down into smaller components. – user441521 Aug 19 '16 at 20:45
  • @Justin catching silly mistakes (as a result of rushing) exactly. rushing in general is a bad practice in software engineering. Robert C. Martin recommends to write code like doing a surgery youtube.com/watch?v=p0O1VVqRSK0 – Jerry Joseph Dec 6 '17 at 23:33
10

In principle, I think it makes sense to prevent people from making changes to the mainline which break the build. That is, the process for making changes to main branch of your repository should require ensuring that all tests still pass. Breaking the build is simply too costly in terms of lost time for all engineers on the project to do anything else.

However, the particular solution of commit hooks isn't a good plan.

  1. The developer has to wait for the tests to run while committing. If the developer has to wait on his workstation for all the tests to pass, you've wasted valuable engineer time. The engineer needs to be able to move onto the next task, even if he'll have to switch back because the tests ended up failing.
  2. Developers may want to commit broken code in a branch. In a larger task, the developers version of the code may spend much time not in a passing state. Obviously, merging that code into the mainline would be very bad. But its rather important that the developer can still use version control to track his progress.
  3. There are occasional good reasons to skip the process and bypass the tests.
  • 2
    #1 is obviated by allowing developers to check-in to a personal branch or local repository. It's only when a developer wants their code somewhere that other developers can see it that the unit tests need to run. As with #1, #2 is obviated by only hooks to mainline branches. #3 is obviated by the fact that A) Any such feature can be disabled, even though it's a hassle (and should be a hassle) and B) Individual failing unit tests can be disabled. – Brian Oct 25 '14 at 18:53
  • @Brian, I'm in total agreement, you can make this work. But, trying to do by blocking the commit hook server-side isn't going to work. – Winston Ewert Oct 26 '14 at 1:03
  • good points. Breaking the build is simply too costly in terms of lost time for all engineers on the project I would suggest using some kind of build notification tool to avoid all engineers ending up loosing time on each broken build – Jerry Joseph Dec 6 '17 at 23:18
3

No, you shouldn't as other answers have pointed out.

If you want to have a code base that is guaranteed to have no failing tests, you could instead develop on feature branches and do pull requests into master. Then you can define preconditions for accepting those pull requests. The benefit is that you can push really quick and the tests run in the background.

2

Having to wait for successfull build and tests on each commit to the main branch is really awful, I think everyone agrees on this.

But there are other ways to achieve a consistent main branch. Here's one suggestion, a bit similar in the vein of gated check-ins in TFS, but which is generalizable to any version control system with branches, although I'll mostly use git terms:

  • Have a staging branch to which you can only commit merges between your dev branches and the main branch

  • Setup a hook that starts or queues a build and tests on commits done on the staging branch, but that does not make the committer wait

  • On successful build and tests, automatically make the main branch go forward if it's up to date

    Note: don't automatically merge into the main branch, because the tested merge, if not a forward merge from the main branch's point of view, might fail when merged into the main branch with commits in between

As a consequence:

  • Forbid human commits to the main branch, automatically if you can, but also as part of the official process if there's a loophole or if it's not technically feasible to enforce this

    At least, you can make sure no one will do it unintentionally, or without malice, once it's a ground rule. You shouldn't attempt to do it, ever.

  • You'll have to choose between:

    • A single staging branch, which will make otherwise successful merges actually fail if a previous yet not build and not tested merge fails

      At least, you'll know which merge failed and have someone correct it, but the merges in between are not trivially traceable (by the version control system) for the results of further build and tests.

      You can look at file annotation (or blaming), but sometimes a change in a file (e.g. configuration) will generate errors in unexpected places. However, this is a rather rare event.

    • Multiple staging branches, which will allow non-conflicting successful merges to get to the main branch

      Even in the event that some other staging branch has non-conflicting failing merges. Traceability is a bit better, least the case that one merger did not expect an affecting change from another merger. But again, this is rare enough to not worry about every day or every week.

      To have non-conflicting merges most of the time, it's important to split staging branches sensibly, e.g. per team, per layer, or per component/project/system/solution (whateven you name it).

      If the main branch was meanwhile forwarded to another merge, you have to merge again. Hopefully, this is not an issue with non-conflicting merges or with very few conflicts.

In comparison to gated check-ins, the advantage here is that you have a guarantee of a working main branch, because the main branch is only allowed to go forward, not to automatically merge your changes with whatever was committed in between. So the third point is the essential difference.

  • But the point of having a working branch isn't (usually) to ensure you have a stable release, but to cut down on the thrashing caused by developers working together in the same code. – Telastyn Oct 25 '14 at 16:11
  • Not if you have a huge repo with big, globally distributed teams. For instance Microsoft uses a similar, more layered approach with Windows. – acelent Oct 25 '14 at 16:15
2

I prefer "passed unit tests" to be a gate for submitting code. However, to make that work, you will need a few things.

You will need a build framework that caches artefacts.

You will need a test framework that caches test status from (successful) test runs, with any given artefact(s).

That way, check-ins with passing unit tests will be quick (cross-check from the source to the artefact that was built when the developer checked the tests prior to check-in), those with failing unit tests will be blocked and developers who conveniently forget to check the builds prior to commit will be encouraged to remember doing so next time, because the compile-and-test cycle is lengthy.

1

I would say it depends on the project and the scope of the automated tests that are run on the "commit".

If the tests you like to run in the check-in trigger are really fast, or if the developer workflow will force some administrative work after such a check-in anyway, the I think it should not matter much and force the devs to only check in stuff that absolutely runs the most basic tests. (I'm assuming you would only run the most basic tests on such a trigger.)

And I think, speed/workflow permitting, it is a good thing to not push changes to other developers that fail the tests - and you only know if they fail if you run them.

You write "commit ... to remote branch" in the question, which would imply to me this is (a) not something a dev does every few minutes, so a small wait may be very well acceptable , and (b) that after such a commit the code changes may impact other devs, so additional checks may be in order.

I can agree with the other answers on "don't make your devs twiddle thumbs while waiting" for such an operation.

1

Broken commits should not be allowed on trunk, because trunk is what may go into production. So you need to ensure there is a gateway they need to pass before being on trunk. However, broken commits can be totally fine in a repo as long as it is not on trunk.

On the other hand, requiring developers to wait/fix issues before pushing changes to the repository has a number of drawbacks.

Some examples:

  • In TDD, it is common to commit and push failing tests for new features before starting to implement the feature
  • The same goes for reporting bugs by committing and pushing a failing test
  • Pushing incomplete code allows 2 or more people to work on a feature in parallel easily
  • Your CI infrastructure can take it's time verifying, but the developer does not have to wait

protected by gnat Jul 26 '18 at 11:15

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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