Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Hypothetical scenario: codebase is exercised by unit tests run by a TeamCity build-bot, that also uses the built-in dotCover tool to provide coverage metrics. The build will fail if less than X% of the code is covered.

Unscrupulous developer running NCrunch (or a pre-tested commit in TC) sees that his next checkin will drop the coverage percentage below the threshold and break the build, because he didn't write good unit tests (TDD or otherwise). So, he writes a new test that runs some lines of code that NCrunch shows aren't covered, but makes no assertions about their behavior. Tests pass by default (because the executed code throws no exceptions), coverage stays above X%, and to find the problem, someone must discover the test, inspect it and see there are no assertions (or no meaningful assertions) made during its execution.

Since we currently don't have a code review process, and it would be detrimental to productivity to perform reviews prior to every commit, I want this behavior to break the build. If the test runner runs a method marked with a [Test] attribute (we're using NUnit) and, upon completion, sees that the code has made no calls to NUnit's Assert methods, nor thrown the ExpectedException, TC should raise Cain. Ideally, the tool would be smart enough to also discover that all assertions will be true by definition, such as Assert.AreEqual(1,1);, and fail the build in a similar way.

Is there something "off-the-shelf" that I can plug into TeamCity, or a way I can configure its built-in runners/coverage metrics to find this type of bad behavior, short of performing a custom static code analysis? Of course we'll find it eventually, but in our environment (small in-house dev team) there may only be one or two developers familiar with the full codebase of a given application, and so this blatant end run around test quality checks may not happen until the guy responsible is long gone and someone else takes over primary ownership of the codebase.

share|improve this question
7  
Wait... why would it be detrimental to productivity to perform reviews prior to every commit? That sounds like a "penny wise and pound foolish" evaluation to me... –  Mason Wheeler Jul 10 '13 at 17:21
1  
This tool works well. –  Robert Harvey Jul 10 '13 at 17:23
2  
@MasonWheeler: Depends on how often commits take place. But given the OP's assertion that the offending developer could be "long gone," I'd say code reviews are not taking place often enough. –  Robert Harvey Jul 10 '13 at 17:25
1  
@KeithS: Code review and pair programming are orthogonal. Whether the code was written by one person or two, whether or not stupid crap like you describe here happens frequently, code should be reviewed by an external pair of eyeballs before it gets committed. The grief you save by catching mistakes early that the developer just plain overlooked--and then not deploying them, not getting bug reports about them and having to come back and fix them when it's no longer fresh in someone's mind--outweighs the time it takes to perform the review. Thus, it's penny wise and pound foolish to skip them. –  Mason Wheeler Jul 10 '13 at 18:04
1  
A dishonest good programmer will be able to circumvent the automatic check anyway. If you have dishonest bad programmers in your "small" team, you have a personnel problem not a technology problem. –  MarkJ Jul 10 '13 at 20:53
show 3 more comments

2 Answers 2

up vote 7 down vote accepted

IMHO If this is happening enough that you are looking for a tool you either:

  • need to reevaluate your policy (you are asking for tests over something that resists testing or the tests provide little value or over a branch that is too early in the process to always have things figured out far enough to write good test harnesses, dummys and mocks)
  • or reevaluate the makeup of your team (testing is appropriate here but your coworkers aren't very good / honest / whatever)

Personally I have experienced far more of the first case than the second case so I generally only examine code coverage when a feature branch is about to be merged back to the master/main.

There are many many more ways to defeat code coverage than there are tools to calculate it or enforce it so this is time that I would choose to spend elsewhere.

share|improve this answer
    
Basically what I'm trying to enforce is TDD. That's really impossible without pair programming or other full-time peer supervision, and there are other obstacles to making that happen in our team room. The next best thing is ensuring the code being written has meaningful tests that exercise its functionality. I do understand the need from time to time to exclude pieces of the codebase for which unit tests are more trouble than they're worth, and that can be done and often is. But, in the pieces that should be tested, I want them tested. –  KeithS Jul 10 '13 at 18:06
2  
Pair programming in this situation would either amount to one of the pair being a test writer or a bunch of infighting. You need to examine the root issue unless you just want to pair each dev with a jr dev minion that only writes tests. –  Bill Jul 10 '13 at 19:55
1  
Agreed, if you are unwilling to invest in synchronous pair programming, you are looking at an asynchronous version like a code review to ensure adequate unit testing. –  neontapir Jul 10 '13 at 22:01
add comment

Only a human is going to be able to tell the difference between a "good test" and a "tautology test".

Even if assertions are included in the test, what's to say they are meaningful? Is checking that the return value is not null enough? Or does the test need to check particular values of the result? Even if the right thing is tested, is the test itself written horribly/confusingly/unmaintainably? And how is an automated tool every going to catch this?

I've run a 30+ person project for years now, and we do code review on every task completion. A task may be multiple commits, but a task is almost always only a few days of work. There is no way that code review "slowed us down".

share|improve this answer
    
hear, hear ! +1 –  Marjan Venema Jul 10 '13 at 19:32
    
Are you saying the reviews are a few days work every sprint? –  Bill Jul 10 '13 at 19:59
    
@Bill Good catch, I meant to say a task is only ever a few days work, even if it is multiple commits. –  Chris Pitman Jul 10 '13 at 20:53
    
so you are doing, on average, 10+ code reviews a day? or you assign ask 30 people to the same task, or I am not understanding you correctly. The way my teams tend to work you would have some really long days just to get 10 dine a day with no other work, and I really cannot imagine 30 people on what I consider a task not stepping on reach other –  Bill Jul 11 '13 at 22:33
    
@Bill Anyone on our team can do code reviews, so it works out to being each person doing one code review every couple days. And no, not everyone works on the same task. I think you are getting hung up on the last paragraph: the original poster was concerned about the velocity drag caused by code reviewing every commit. I simply meant that we do that on my team, but instead of every commit we review groups of commits that accomplish a task. –  Chris Pitman Jul 11 '13 at 23:27
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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