Do you code review unit tests?

Question

After reading this question I'm left wondering if shops doing code reviews bother to do the unit tests as well?

At my current contract all code must be reviewed before commit, but historically unit tests have not been taken seriously. This is an area I'm addressing at the moment and was wondering if people would include the unit tests with the review and if so, how you might address doing this where the developers experience of unit testing is quite low, with some of them having zero experience at all?

Answer 1

Statistically, unit tests are more buggy because they ARE taken less seriously. So yes, code review those buggers.

Answer 2

Yes. It's quick. A test should be obvious; if it isn't, you don't have a trustworthy test case.

One team member was over-testing (he was unit testing test data for other unit tests.)

It's a way to check coverage, style, and general correctness.

Also, unit test defects aren't as serious as working code defects. If there are style issues, you don't have to debate the merits of spaces around ()'s again. You just say "this has style problems on lines 43, 87, 96, but we're committing it anyway."

You should have a backlog of unittest bug fixes just like any other bug fix.

Answer 3

If you are going to bother to unit test, and you are going to bother to do code reviews, then you really should also bother to review the unit tests.

For me, if you are going to take unit testing seriously, your test code is as important as your application code.

If your developers experience of unit testing is low, then they need feedback on how they should be doing it, whether that is in the form of a code review or some other mechanism.

I would recommend pair-programming between one developer with unit test experience, and a novice, and then review that with the code reviewer being a third person.

Answer 4

At my place of work, we recently started taking unit-test more seriously. We are currently defining a list of items to go over in our code reviews, and one of these items is the unit-tests.

Right now, this item just means the existence of unit-tests. Once we get over this, we'll start reviewing the tests themselves more seriously. Not right now, though.

Answer 5

I agree that the unit tests should also be reviewed. As has been mentioned by others, too many developers skimp on unit testing and testing as a whole and by reviewing them you can only improve on the testing performed and identifying areas that require unit testing and what sort of testing.

Answer 6

The code written for the unit tests usually is a lot more simpler than the production code (no if, no loop, test only one thing ...). So the unit test code is less error-prone and we don't review it formally.

Also, we strive to use Test-Driven Development (TDD), so we first run all of our scripts so that they fail first, and then we write to the code to make it pass. This add to the quality of our tests although I have to admit we sometimes had issues in the unit tests.

Having the developers review the unit tests code will show them that unit testing is not that difficult and help them getting started.

One way to review the all unit test code as well as all the production code is Pair Programming, although it's hard to get both management and developers buy-in.

Like production code, the unit tests code shall be maintained as well, so one shall pay some attention so that it is kept readable and maintainable.

Answer 7

We don't formally peer review unit tests, but they are the first place that I start when reviewing another person's code. I only bring it up in the review meeting if I find something specific wrong with a test, or it they're testing too little or too much.

(Yes, you can test too much. I've seen unit tests that check to make sure a constructor is returning an object of the correct type.)