2015-05-21

It's OK to submit patches without tests: just show the correctness proofs instead


Bruxelles

In a window adjacent to the browser I'm typing this, I'm doing a clean build of trunk prior to adding a two line patch to Hadoop -and the 20+ lines needed to test that patch; Intellij burbles away for 5 minutes as it does on any changes to the POMs on a project which has a significant of the hadoop stack loaded.

The patch I'm going write is to fix a bug introduced by a previous three line patch, one that didn't come with patches because it was "trivial".

It may have been a trivial patch, but it was a broken trivial patch. It's not so much that the test cases would have found this, but they'd have forced the author to think more about the inputs to the two-line method, and what outputs would be expected. Then we get some tests that generate the desired outputs for the different outputs, ones that guarantee that over time the behaviour is constant.

Instead we have a colleague spending a day trying to track down a remote functional test run, one that has been reduced to a multi-hop stack trace problem. The big functional test suites did find that bug (good), but because the cost of debugging and isolating that failure is higher handling that failure is more expensive.; With better distributed test tooling, especially log aggregation and analysis, that cost should be lower —but it's still pretty needless for something that could have been avoided simply by thinking through the inputs.

Complex functional system tests should not be used as a substitute for unit tests on isolated bits of code. 

I'm not going to highlight the issue, or identify who wrote that patch, because it's not fair: it could be any of us, and I am just as guilty of submitting "trivial" patches. If something is 1-2 lines long, it's really hard to justify in your brain the effort of writing the more complex tests to go with it.


If the code actually works as intended, you've saved time and all is well. But if it doesn't, that failure shows up later in full stack tests (cost & time), the field (very expensive), and either way ends up being fixed the way the original could have been done.

And as documented many times before: it's that thinking about inputs & outputs that forces you to write good code.

Anyway: I have tests to write now, before turning on to what is the kind of problem where those functional tests are justified, such as Jersey client not working on Java 8. (I can replicate that in a unit test, but only in my windows server/java 8 VM)

In future, if I see anyone use "trivial patch" as a reason to not write tests, I'll be wheeling out the -1 veto.

I do however, offer an exception: if people can prove their code works, I'll be happy

(photo: wall in Brussels)

No comments:

Post a Comment

Comments are usually moderated -sorry.