7

Note: after writing this I realize that this question is perhaps philosophical, but I'm interested in how the industry handles this scenario regardless.

I have recently been working with a code base that is not yet as testable as it should be. I've seen some junior developers check in code that breaks use cases, which is usually identified by another developer a few days after the check-in.

Assuming that we can't achieve code coverage yet, are there technical solutions for holding developers accountable for bad check-ins in addition to a non-technical approach; discussing the commit and having the dev address the issue in a new commit.

7
  • 1
    How do you find out the check in broke the code? Manual testing? Unit testing? Some other dev found out? It all depends on this.
    – Euphoric
    Dec 27, 2012 at 22:09
  • 2
    between "not yet as testable as it should be" and "hold developers accountable" this wreaks of bad code that developers are having trouble maintaining and you feel the stick is the answer. How about before trying to hold them accountable for the bad code that came before them, you allow them to make the code testable. All the developer-beatings in the world won't help them stop breaking things when the code is confusing and overly fragile. Dec 27, 2012 at 22:13
  • 1
    instead of the stick provide some carrots to refactor parts of the code base to make them more testable Dec 27, 2012 at 22:31
  • 7
    The answer is that you should do Code Reviews... 'nuff said.
    – Spoike
    Dec 27, 2012 at 23:03
  • 4
    I have found the term "accountability" to be a buzzword these days. What does accountable mean? How do you expect a technical solution to hold them "accountable"? Will a robot be dispatched to go shoot whoever checked in the code? Dec 28, 2012 at 3:10

6 Answers 6

3

Two simple measures:

  1. Code reviews (already mentioned by others) before each check in. You do not need to explain every detail of what you check in, but the reviewer should get an idea of the changes that are being checked in. You can program your RCS to reject check-ins whose check-in text does not contain a tag "REVIEWED BY: ...".
  2. Agree with the rest of the team that if a check-in breaks any unit tests, the developer who has performed the check-in must fix the code until those unit tests are green again (possible measure: write a bug and assign it to him / her). As an alternative: if a team member detects a check-in that breaks the unit tests, he or she can revert the changes. The author of the wrong check-in is not allowed to check-in again as long as the problem is not fixed locally (unit tests are green locally).

NOTE

Point 2 has been edited taking into account gnat's comment.

5
  • if I would be "the rest of the team" I would think twice before blindly agreeing on #2. It could easily happen that forcing things to be fixed by "developer who performed check-in" will block my own work. In one of past projects I wasted more than a month expecting for this to happen; after that I fixed it myself in less than a day. Don't want to break my nose like that again
    – gnat
    Dec 28, 2012 at 11:36
  • @gnat: As an alternative: revert their check-in and forbid them to check-in again until they have all unit tests green on their machine. In this way they do not make any progress with their work until they fix the problem. You can detect a wrong check-in within minutes if you use continuous build.
    – Giorgio
    Dec 28, 2012 at 11:39
  • yup, only your answer doesn't mention this alternative - that's why I wrote I'd think twice...
    – gnat
    Dec 28, 2012 at 11:46
  • ...note also that since per your #1 reviewer shares responsibility for (and more importantly, shares understanding of) the commit with the author, they can also be assigned to fix/rollback the breaking change. Less of a bus factor, better for teamwork
    – gnat
    Dec 28, 2012 at 12:06
  • 1
    @gnat: Good point, even though the original author of the code would still be the primary candidate for the fix. But yes, the reviewer is the next candidate, e.g. if the author of the check-in is ill for a couple of days, the reviewer is the next best person who knows what is going on in that part of the code and who should try to fix it.
    – Giorgio
    Dec 28, 2012 at 12:08
3

If you use git for version control check out gerrit. Sure it can be a pain in the ass, but it does require code reviews to take place before a commit is merged into the repository.

4
  • +1 very interesting. Surely I'll try to use your advice.
    – Gangnus
    Dec 27, 2012 at 22:35
  • You might have to relearn the way you use git. Here's a helpful command. git pull --rebase Gerrit loves rebase Dec 27, 2012 at 23:56
  • I have to learn the way of use both git and gerrit. I haven't used them yet :-). We use svn+fisheye+bamboo+JIRA now. But we want much more code reviews and some control will be useful
    – Gangnus
    Dec 28, 2012 at 0:19
  • @Gangnus: Jira is good for complex code reviews, but for relatively small, self-contained changes I find that reviewing the code directly at the terminal is way more efficient.
    – Giorgio
    Dec 28, 2012 at 11:35
3

You need to implement a Continuous Integration (CI) system like TeamCity. It should not be some developer a few days later noticing the problem. With a CI system as soon as a check-in occurs the code is automatically built and the automated tests are executed. Within minutes of the check-in the CI system will alert everyone that the build has been broken and who did the check-in that caused it.

This way it is all completely objective and there is no finger pointing about who is at fault. Unlike developers the CI system is using only that latest code from source control to do its testing. There are no local changes and no funky configuration differences. Most systems have a tray icon that turns red when the build is broken. All developers should have that icon installed and keep track if the build is OK or broken.

1
  • 1
    I think the problem is the lack of automated test cases.
    – arin
    Dec 28, 2012 at 16:46
1

I think your team needs to attend an agile scrum course and make a pact to follow the methodology.

When developers check-in small code changes assigned to small tasks it reduces the chance of developers going off the rails and checking-in conflicting code. This increases continuous integration.

"I've seen some junior developers check in code that breaks use cases, which is usually identified by another developer a few days after the check-in."

Even in a perfect world this still happens. If you find its happening frequently and/or after several days of development then you have a problem.

I think you'll find the team will work together more effectively with a planned iteration. I suggest you try out a 2 week sprint. Start the sprint with an in-depth scrum meeting. Go through the backlog of Bugs and Work Items, accepting the top priorities and writing them down on post-it notes with units/time estimates using planning poker cards.

Line up a scrum dashboard or white board and ensure people only work on one thing at a time. Once they have finished the task they need to write a unit test for it. Once they write a working unit test they will need to get a Code Review. Constructive code reviews is one of the best techniques to prevent developers accidentally checking-in code conflicts.

After the Code Reviewer has signed off, the developer does the Check-In Dance:

  • Let the rest of the team know a change is coming if it’s a significant update.
  • Get the latest code from source control.
  • Do a merge on any conflicts.
  • Run the build locally and fix any problems found.
  • Run the Unit Tests locally and fix any problems found.
  • Commit the changes to source control.
  • Stop coding until the build passes.

If the build breaks, drop everything else and fix the build.

Team Foundation Server (TFS) faciliates all this stuff out-of-the-box.

You should fix up the tests before moving forward. But just because 80% of code is unit tested that doesn't mean its bug proof. Developers have to be active in testing their code sociability. This code integration is much easier to achieve using non-waterfall methodologies.

0

I second code reviews. Some tools let you do reviews before the code is committed like Github pull requests. Others let you see who touched each line like SVN blame.

Personally, I'd rather my team members commit code to the trunk frequently even if it isn't perfect. It increases the chances someone else will notice an issue and allows for the benefits of continuous integration.

We do formal code reviews at the end of a task. I also do "random" code reviews as people work and commit. The random is in quotes, because I review more heavily at the beginning to train people well how to test. I'll also offer to pair to test so developers gain the skills. The random is also in quotes because people who write worse code/tests get chosen more often for the random reviews. I keep the random reviews anonymous - management never hears about them - they are a purely team practice.

0

Either of the following should reduce the risk of a junior committing bad code drastically :

  • Pair programming, preferably with a senior developer as a navigator

  • Pre-commit code reviews by a senior developer

A nice thing about these is that they are collaboration-oriented, not blame-oriented, which IMO is definitely something you should favor when integrating junior programmers.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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