Let's clean our tests up

Unknown
Unknown
in Dev & Ops

Looking at our PHPUnit output from current vanilla master I see:

...............................................................  63 / 765 (  8%)
............................................................... 126 / 765 ( 16%)
............................................................... 189 / 765 ( 24%)
...........................................................S... 252 / 765 ( 32%)
...........S............SS..................................... 315 / 765 ( 41%)
............................................................... 378 / 765 ( 49%)
............................................................... 441 / 765 ( 57%)
............................................................... 504 / 765 ( 65%)
............................................SSSS..S............ 567 / 765 ( 74%)
.......................................S............S.......... 630 / 765 ( 82%)
.S.....S..........S..............S.......SS.SS..S.............. 693 / 765 ( 90%)
..........SSS.SSSSS.SS..SSSS.......S........................... 756 / 765 ( 98%)
.........                                                       765 / 765 (100%)
Time: 1.22 minutes, Memory: 46.00MB
OK, but incomplete, skipped, or risky tests!
Tests: 765, Assertions: 2984, Skipped: 35.

Our tests are passing, but there are a lot of skipped tests. To me, a skipped test is something that should either work in some context or be fixed. There is also an incomplete flag we could use to mean that the functionality or test has not been developed yet.

In our own code there some of these tests are marked as skipped because we will never implement the functionality or not any time soon at least. In these cases we should remove the tests completely. A lot of the skipped tests result from inheriting a test base class that provides boilerplate tests that not all subclasses need. This is okay, but we still need to address this.

Here is what I propose:

  1. Any test that is a result of functionality that should be reasonably be expected to be implemented in the future should be marked incomplete. Incomplete tests should get into our sprints to be cleaned up in the future. Examples of these are some APIv2 endpoints like DELETE /conversations/:id.
  2. Only skip tests that can't be run because of the environment. This would be tests like any Sphinx plugin tests when Sphinx isn't installed.
  3. All other tests should be removed. If they can't be removed because of subclassing I've implemented a solution. You can add @group ignore to the test's docblock and our travis build will ignore it. Also from the pull-request, tests that are going to be run by very few of the subclasses can be moved into traits and used only where they are needed.

The main reason I want this is so that we can see that our build is working. As we add more and more unit tests having a bunch of skipped tests could indicate bugs, but we won't know where because there are so many of them.

Categories