On Unit Testing Requirements

My last post drew quite a few responses and got me thinking even more about our current unit testing policy. Right now the toolkit review requirements (and the browser review requirements) state the following:

• All patches must include unit tests which are run during make check. Once unit tests are committed, the in-testsuite+ flag should be set on the bug.
• Unit tests must be included in patches and reviewed just like any other code.
• If running the test during make check is not possible, consult with Robert Sayre to determine whether another test system is available.
• If a unit test framework is needed but is not yet available, the developer should write appropriate test code and commit it. A bug should be filed on the needed test framework. The in-testsuite? flag should be set on the bug until the framework has been completed and the test code is running automatically.
• Certain build-config or tooling bugs which don’t affect the actual product cannot be usefully tested. These bugs should have the in-testsuite- flag set.

I’m quoting that because I don’t think many people (especially based on the comments I got) are even aware of this policy. I suppose that this is the case because nobody’s really enforced it before – it’s been more of a suggestion that people should try to follow. In fact, there are several modules in toolkit and browser that have little to no testing (especially of the UI) that are actively being worked on (I’m not trying to point fingers here). This brings up a good question: “Why isn’t this policy being enforced at least somewhat uniformly?” I think part of the problem is that people feel that it’s a waste of their time, and that it won’t actually bring any benefit (I’ll save the argument about how wrong this viewpoint is, especially when we are trying to ship a release for some other day). Another problem is that this policy, as written, is a bit unrealistic.

I think Rob has the right idea about not requiring tests from new contributors. However, the current policy doesn’t allow for that as an exception, so someone else needs to write the test. One commenter suggested that someone else write the test(s) for the patch submitter. From a patcher’s perspective, that seems like a great idea. However, who would end up writing those test cases? Most likely, that would fall onto the module owner, or if he/she is lucky, one of the peers (I should note that Edward and I have written many tests for various bugs that we didn’t write the patch for, so this does happen sometimes). Most module owners and their peers are often patchers as well. If they have to now write tests for other people’s patches, do reviews in their module, and do patches elsewhere (because nobody who’s a peer or module owner really works in just one module), they are highly likely to put something on the back-burner. Personally, any bug that wasn’t a serious issue that required me to write a test would probably become very low priority (and I suspect that to be the same for most other people). In the end, it’s just shifting the burden to people who are already very busy.

I think it’s fairly clear that we have a policy that isn’t in line with most individual’s expectations. Or maybe we, as submodule owners, aren’t supposed to be so strict about it and I just didn’t get the memo (hey, I’m new to this, so it’s possible). Regardless, I feel that something should be done about the situation.