Home > Best Practices > You shall not commit! (Without passing tests)

You shall not commit! (Without passing tests)

January 24, 2012

Welcome to the SmugMug engineering blog! Here at SmugMug we refer to our engineers as ‘Sorcerers’ and this blog will be all about the magic behind smugmug.com. Our engineering landscape is as vast and varied as Middle Earth so we’ll start with something everyone’s familiar with: committing code and testing.

At SmugMug our SCM tool is Bazaar (soon to be Git) and the unit test framework we fancy is PHPUnit since our website codebase is written in PHP. Running unit tests before committing is always a Good Thing for developers to do, but sometimes we forget and can commit code with broken tests. This is where a pre-commit hook comes in handy.

A pre-commit hook is a piece of code/script you tell your SCM tool to run after you type `bzr/git/svn commit` but before it actually commits your new code. A pre-commit hook can do just about anything, and with ours we run our unit tests and if they fail, we abort the commit.

The way we prevent ourselves from committing code with broken tests is by creating a short shell script that runs our unit tests via PHPUnit’s command line tool. Our script is named `precommit.sh’ and is pretty simple:

#!/bin/sh
phpunit --bootstrap include/tests/bootstrap.php include/tests/

That script runs from the root of our Bazaar repo, telling PHPUnit where our bootstrap file and tests are. The script will return 0 if all tests passed (in Unix 0 means the command succeeded). If tests fail the script will return something other than 0. Now that we have that working (and our tests pass :)), we can get Bazaar to run it before committing our code.

The way you get Bazaar to run a pre-commit hook is by writing a plugin that adds the hook to Bazaar every time you run `bzr commit`. Plugins are simply Python files placed in ~/.bazaar/plugins/[plugin-name].py . Bazaar executes them and the plugins can then do just about anything they please. Here’s our pre-commit hook plugin, which I’ll walk us through:

from bzrlib import branch

def check_tests(local_branch, master_branch, old_revision_number, old_revision_id, future_revision_number, future_revision_id, tree_delta, future_tree):
    import os,subprocess
    from bzrlib import errors

    #Only execute our script if it exists in the current directory
    if not os.path.exists("precommit.sh"):
        return

    #Run the pre-commit script
    tests_result = subprocess.call(os.path.abspath("precommit.sh"), shell=True)

    #If our script returns something other than 0, tests have failed
    if tests_result != 0:
        raise errors.BzrError("Tests failed, no soup for you!")

#Install the hook with Bazaar
branch.Branch.hooks.install_named_hook('pre_commit', check_tests, 'Run PHPUnit tests pre-commit hook')

Our plugin does the following:

  • Imports ‘branch’ from the Python module bzrlib.
  • Defines our function to run before committing new code. A pre-commit hook function accepts eight arguments, none of which we’ll need though. (For more info on hooks with Bazaar, read the hooks help page)
  • Checks for the existence of precommit.sh, which runs our tests. We do this because we could be working with multiple repos, some which may not have a pre-commit script to run.
  • If it does not exist we return and Bazaar will commit our code.
  • If it does exist we run precommit.sh via subprocess.call() and save the return code as ‘tests_result’.
  • If tests_result is is not 0, our tests have failed and we raise a Bazaar error, which will abort the commit.
  • Installs the pre-commit hook via branch.Branch.hooks.install_named_hook(). The first argument is where the hook should be run, the second is the function to run and the third is a name for the hook to be used in progress and error messages.

Here’s what it looks like for us if we commit code with broken tests:

With just a little bit of engineering we can have our own wizard preventing us from committing broken tests and code! Most other SCM tools have pre-commit hooks also (Git, SVN). Stay tuned for more posts about our architecture and engineering processes here at SmugMug.

  1. January 24, 2012 at 3:06 pm

    Thanks for the info.. I will pass this on to my dev team

  2. January 24, 2012 at 3:07 pm

    Oh my, “committing” brings back so many memories… Live by the motto!

  3. January 24, 2012 at 3:39 pm

    black / green terminal – how very h@x0r of you guys 😛

  4. January 24, 2012 at 4:10 pm

    You should look into Continuous Integration servers as a much more flexible and reliable solution.

  5. January 24, 2012 at 4:16 pm

    @Rudi: Yep, that’s an excellent point. CI is an area we’re working on too. This is a quick and easy way to get the ball rolling with automated tests 🙂

  6. January 24, 2012 at 6:40 pm

    This is great. If only more web development teams took the time to be so thorough. I look forward to those future posts you mentioned 🙂

  7. January 24, 2012 at 8:20 pm

    I’m certain I have no idea what you all are talking about… Yup!

  8. January 24, 2012 at 11:20 pm

    Are you doing this for all commits? Or only on the stable branch (from where you make releases)?

    Although I’m in favour of all kind of ways of testing mechanisme, it seems a little bit hardcore? I suppose you guys have 100 if not 1000 of tests, which can take a long time to run? And so the dev will have to wait a long time for his commit to be finished!?

    As @Rudi mentioned, Continues Integration is the way to go. Personally I love Jenkins, which works very well with php+phpunit.

    But hey, that’s only my point of view.

    Keep up to good work.

    Smugmug Lover

  9. January 24, 2012 at 11:42 pm

    SmugMug is awesome! Heroes, Sorcerers and the whole gang! Always innovative and delivering great new features with a fresh twist of amusing lingo that makes me smile! Keep it coming! For now, I will remain a photographer and let the Sorcerers do their magic!

  10. January 25, 2012 at 9:14 am

    @bmagix: It’s up to the developer to decide what branch(es) they want all tests to run. Personally I don’t run all tests until I’m committing to trunk or a major feature branch, tiny or short lived branches don’t do this.

    We’re working towards CI, this was a quick and easy way to get ourselves in the ‘always test’ mindset 🙂

  11. January 25, 2012 at 10:06 am

    @Ryan Oh right, I see. I missed the point (I need to learn to read correctly lol) where the precommit.sh is commited within project itself and so configurable by the dev for its needs. So for example if the dev writes 1 new Test class he can decide to only configure that test in the precommit file?

    Interesting hack for small unit testing 🙂

  12. January 25, 2012 at 12:08 pm

    Very cool. FYIW, I’d love to see the ability to subscribe to future blog posts via email (akin to the method used on the primary new.smugmug.com blog).

    • January 25, 2012 at 1:55 pm

      @Cody Just added the Subscribe-by-Email option, sorry we forgot about it earlier!

  13. Boris
    January 25, 2012 at 10:09 pm

    As already other guys commented – continuous integration by checking ALL commits is the way.. To check only some commits and some not is bringing possible troubles for the future..

  14. January 26, 2012 at 5:32 am

    Love the Seinfeld reference…”No soup for you….!” Who says geeks have no humor?

  15. January 26, 2012 at 11:38 am

    Cool, I’m looking forward to seeing what else your sorcerers are up to. I’m interested in hearing about why you’re transitioning from bzr to git. (Personally, I love git but haven’t experimented with bzr).

  16. January 30, 2012 at 1:11 pm

    Guys –

    I’m new to your Blog but I’ve been having a continuous issue with my Slideshow function. I’ve included my latest E-Mail to the “Heroes” Group and they suggested that I post here. Here is a statement of the problem: “I’ve shut off all Watermarks in my Galleries. However, when you start a Slideshow that incredibly lousy Smug Mug “PROOF” continues to show up part way through the show. How do I keep that Watermark from appearing?” Now this issue has been going on for over a year and I’ve totally worn out about five sets of “Heroes”! Doc found a “Bug” in the Code over a year ago and indicated that it was passed to you guys for resolution. Still no joy!! I re-sent it again last week to the “Heroes”. Again Doc identified the issue, and again it was passed to you guys to work the problem. I need help here!! Please review the very extensive E-Mails that have been floating back-and-forth between me and the “Heroes”. Thank you for your help with this issue!

    – Vic

  17. January 30, 2012 at 1:29 pm

    @Victor: Sorry you’re still having problems! I’ve contacted our Heroes and they are tracking the issue you reported to them. In the future it’s still best to contact them for any issues. Thanks!

    • January 30, 2012 at 4:27 pm

      Ryan –

      Not so fast. I feel like a ping pong ball here. I was working with the Heroes and they sent me to you. Now you’re trying to push me back to them. Someone at Smug Mug needs to take ownership of this problem. This situation has been going on for over a year now, and I still don’t have a resolution. There is a “bug” in the code. Everyone seems to agree on that. This needs to be fixed. I am not happy about being handed-off again.

      This needs to be bumped up the chain to your Supervision.

      Vic

      • January 31, 2012 at 1:22 pm

        Victor,

        We are actively looking into this issue at the moment. I agree that this issue has been open for a long time, but juggling priorities between new features and fixes is not simple and sometimes issues fall through the cracks. Sorry that this happened to be one of those issues.

      • January 31, 2012 at 2:12 pm

        Miikka –

        Thank you for your response. I also understand something about priorities. This issue has been one of mine for a long, long time. I also understand that no one wants to tackle this kind of a fix; it’s intermittent, very frustrating to sort out and understand the potential causes, and I may be the only one in the World with this problem.

        However, I am glad that perhaps now I will be on a “front burner”. I assume that someone in the organization has been charged with the responsibility of keeping me informed as to the resolution progress.

        Thank you.

        Vic

  18. August 21, 2012 at 8:28 am

    Nathan Gehman :
    Love the Seinfeld reference…”No soup for you….!” Who says geeks have no humor?

    +1

  1. No trackbacks yet.
Comments are closed.
%d bloggers like this: