One of my recent larger projects at work was to improve our pretty large PHPUnit test suite, which consists of nearly 7000 test cases. While accomplishing this useful but exhausting work, I made my notes of the most common bad coding, architecture and setup patterns, and this compilation I’d like to offer to you. Mostly, the examples refer to test code, although some were a consequence of poor design decisions. The order in which I list the bad practices and errors, is not the order of their importance. Mostly, it is the order of occurences.
- Lower error_reporting level for unit tests. The first stage of improvements was a task that sounded fairly simple: turn on E_WARNINGs for unit tests. Yes, we used to have them off, as well as E_NOTICE and E_STRICT. And you know what? Never. Repeat. This. Error. I mean it. We have a tough code review process established, and the engineers are quite experienced. That’s why I did not expect too much trouble with this. I was wrong. In test/bootstrap.php I changed error reporting, ran the test suite, and I was totally frustrated by the number of warnings in the report: seven damn hundred! Fixing those was mostly a long and boring job of filling in the missing arguments for function/method calls. Yet I felt really bad about this: fixing a missing argument could introduce another error, and probably even harder to track. If we had error_reporting of E_ALL from the very beginning everywhere, this situation could never happen. What is worse is that a warning or even a notice is somewhat likely to be just the top of the iceberg, with any sort of weird logical error lying deeper. A particular such case is listed below under number 2. In the end, after long days of work, I managed to eliminate all warnings. However, E_NOTICE and E_STRICT are still hidden, and I can only hope it will not be my job to deal with them…
- Forgotten dataProvider annotation. In several test methods, the dataProvider annotation was missing. Method has signature like this: function testSomeBehavior($input, $expectation), and there is a method below called testSomeBehaviorProvider, but PHPUnit is not told about data provider. Hence, PHPUnit calls method as usual, without arguments, which in PHP is equivalent to passing NULLs, though an E_WARNING is raised. Also, those test methods were only executed one time, instead of several times with different arguments. Those tests were written in such way that giving all NULLs as input was actually OK and all the assertions passed. When writing, developer probably ran a whole test folder, saw the green line and moved on. It is also easy to not pay attention to such a mistake during code review. However, simply turning E_WARNINGs on would have made this impossible.
- Static state manipulation. Some people say globals are evil but don’t care about singletons and alike. When using (and especially when modifying) static state, you better ask yourself twice if you really need it. If there is no other way, be extremely careful. You change the contents of a static class member — it’s left there for all the future users of it. You have to make it 100% sure that you restore the member to its initial state after you’re done, no matter how your test method is terminated, be it an exception or a return. Consider this:
DatabaseConnection::$instance is a DB object.
In your test, you change it:
DatabaseConnection::$instance = new DBMock();
Then, all the other test methods, those that will be called later in the test suite and that expect a DB instance, will meet something unexpected, a mock! Maybe you thought about this, so before return in the test method you restore the value:
DatabaseConnection::$instance = $originalDB;
However, it might happen that an Exception is thrown before you restore original value, and those situations must be taken care of, as well. The consequence of this particular error is hard-to-debug weird behavior. What makes it even worse is that the weirdness is observed later in a completely different place. The conclusion: don’t mess with statics. If you have to, be extremely careful! - Using implicit fixtures in functional tests. In our terms, functional test is one that talks to real database, not to a mock. It happened (yes, “historical reasons”, ya know) that for unit tests we use the same DB snapshot as for development. That is, it contains a small portion of real data with sensitive information obfuscated. I found a lot of functional tests follow this pattern: 1) get an object by ID; 2) apply certain actions; 3) verify the resulting DB structure. For example:
$order = find_order(3456789);
cancel_order($order);
assert(order_is_cancelled(3456789));
The motivation behind this would be like “For my test, I need an order of type X with 2+ products in it. Let me find that in our DB snapshot… Ahh, here you are, order ID 3456789”. There are two major problems with this approach. First, those numbers don’t say anything to anyone. Much better would be to create a new order in setUp() or in the test method. Learn DbUnit, it’s a handy way to test against a real database. Second, your DB snapshot is not really stable, and you may find your test failing without any of the relevant code changed. This can be frustrating and take long time to find out what’s happening. Also, if you don’t have BEGIN … ROLLBACK in your setUp() and tearDown(), then it’s even worse. For that, see point 5: - Not cleaning up after your dog. Functional tests read and write to the real database. For proper isolation, the DB state after each test should always remain the same. Unchanged. This should be easily achievable by BEGIN … ROLLBACK in your setUp() and tearDown() respectively. In our case, we did have this in place… Occasionally, it used to be broken in certain cases (see point 3: mangling with statics). This stuff caused me a lot of trouble when I was to introduce parallel test execution in order to speedup things. The reason being that if your tests modify the database, it is obvious that they rely on a certain order of test execution. I introduced concurrent test runner and got a lot of failures. Test A that asserted to have X rows in some table started to fail because of test B doing INSERTs to that table. Before concurrency, test A used to always run before B, and it all seemed OK. I ran the test suite with several workers — and it screwed everything up. In the end, the cure for pains 4 & 5 is: make your functional tests isolated by running every test case in a transaction which is then rolled back; create explicit fixtures (hello DBUnit!).