Is it a good practice to run unit tests in version control hooks?

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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?

Answer 4

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.

Answer 5

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.

Answer 6

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.

Answer 7

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.

Answer 8

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.

Answer 9

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