6

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.

Improve this question
7
  • 16
    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, 2013 at 17:21
  • 4
    @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, 2013 at 17:25
  • Code coverage with no asserts isn't worthless.
    – user16764
    Jul 10, 2013 at 17:43
  • 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, 2013 at 18:04
  • 7
    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, 2013 at 20:53

3 Answers 3

Reset to default
10

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.

Improve this answer
3
  • 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, 2013 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, 2013 at 19:55
  • 2
    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, 2013 at 22:01
17

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".

Improve this answer
6
  • 3
    hear, hear ! +1
    – Marjan Venema
    Jul 10, 2013 at 19:32
  • Are you saying the reviews are a few days work every sprint?
    – Bill
    Jul 10, 2013 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, 2013 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, 2013 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, 2013 at 23:27
6

Mutation Testing

The OP mentioned that they're trying to enforce TDD. With TDD, the only production code written is that necessary to make tests pass, so in most cases changing that code should cause at least one test to fail (in Mockist TDD ideally exactly one test should fail).

A mutation testing tool will automatically make many small changes to the code, such as replacing + with - or deleting a statement, creating many mutant variants of the codebase. It will run the full test suite against each variant, skipping any tests that don't cover the mutated lines.

You can set your CI system to require a certain minimum percentage of mutants to be detected by the test suite.

Improve this answer
1
  • 1
    +1 This is a great answer. Mutation testing is a good way of "testing the health of the tests themselves".
    – Andres F.
    Dec 27, 2019 at 17:44

Your Answer

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

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