Slides for my talk “Scaling Communication via Continuous Integration” are now available on Slide Share.
As presented at PHPUK2012 in London.
New Addition to CSRunner: php -l
Speed is addictive.
After speeding up PHP Code Sniffer runs, the slowest job for Etsy was running php -l on all the files.
So, in short, the latest release of CSRunner, version 0.2.1, will execute lint on the recently changed files.
You can also run the lint command separately, but why would you want to do this instead of your precious find piped to a php -l script?
Because this command outputs to checkstyle format!
Which means if you are Jenkins and using the Jenkins Checkstyle Plugin, you can add the resulting checkstyle xml to your list of Checkstyle results. Voila!
Enjoy!
PHP CodeSniffer for Recently Changed Files
At Etsy we have been running PHP_CodeSniffer as part of our pre-commit. When we ran on the entire code base it took at least 5 minutes, which is too slow for the Etsy Continuous Deployment pipeline.
Immediately suggestions flew in to only run the sniffs on the changed files, but you can’t just define the set of changed files as files that have changed since the last run.
Why not? Because the set of changed files in the next run will not necessarily contain the files that changed. In fact, the clever programmer will watch the code sniffer find errors in his code and will then push the build button again to watch it turn green on an empty change set.
The elegant solution around this would be to maintain a dictionary of files that have violations in them. The Jenkins (or other CI system) job would run phpcs on changed files and the files that were listed in the dictionary of files that have violations. The job would then alter the dictionary to contain the files that are still failing, but maintaining dictionaries is too tedious to satisfy the minimums that we need.
We minimally need a Jenkins job that takes under 5 minutes to run phpcs on all files that have been recently changed without missing file that has a violation.
The solution, in Git:
-
Find a SHA that was recent enough to not include too many files, but old enough so that there has definitely been at least one change.
git log --format='%H' --since='-5 days ago' | tail -n1 -
Now get the list of the files changed since that SHA
git diff --name-only {$old_sha}..HEAD | sed '/^$/d' | uniq - Pass that list to
phpcs
Five days is long enough to account for very long weekends, but for the most part there is always someone reason to have a deployment per day. With the five day window we see runtimes of maybe a minute now.
This solution is not complete however because if you pass phpcs a list of files to run on, then the ignore patterns are ignored. In other words, you have to do the blacklisting of files, sub-directories, etc. on your own.
Ideally, I should probably look at PHP_CodeSniffer to fix this, but in the interim, I give you CSRunner.
CSRunner simply requires an implementation of the Scm interface, and whatever implementations of the Filter interface you so desire. I started with giving it a Git implementation of the Scm that let’s you pass a remote, branch, and a time window for changes, and a Filter implementation that allows you to list patterns as you would in your ruleset.xml to ignore named Blacklist. Rather then wasting so much effort on config files, formats, and parsers, the README gives you nice example of how to write a little script to utilize this code. Honestly, I feel you all can handle it.
An Example: Avoid Private Functions in Unit Tests
@__edorian writes: @elblinkin I can’t seem to make the BowlingGameTest fit your coding standard: [gist here] How does that work out for you?
Here is the code he was talking about:
<?php
class GameTest extends PHPUnit_Framework_TestCase {
/**
* @var Game
*/
private $game;
protected function setUp() {
$this->game = new Game();
}
public function testGutterGame() {
$this->rollMany(20, 0);
$this->assertSame(0, $this->game->score());
}
public function testRollAllOnes() {
$this->rollMany(20, 1);
$this->assertSame(20, $this->game->score());
}
public function testRollSpare() {
$this->rollSpare();
$this->game->roll(1);
$this->rollMany(17, 0);
$this->assertSame(12, $this->game->score());
}
public function testRollStrike() {
$this->game->roll(10);
$this->game->roll(1);
$this->game->roll(3);
$this->rollMany(16, 0);
$this->assertSame(18, $this->game->score());
}
public function testPerfectGame() {
$this->rollMany(12, 10);
$this->assertSame(300, $this->game->score());
}
public function testThenStrikesThenTwoFives() {
$this->rollMany(10, 10);
$this->rollMany(2, 5);
$this->assertSame(285, $this->game->score());
}
private function rollSpare() {
$this->game->roll(5);
$this->game->roll(5);
}
private function rollMany($rolls, $pins) {
for (; $rolls; --$rolls) {
$this->game->roll($pins);
}
}
/**
* @expectedException InvalidArgumentException
*/
public function testRollingNegativeAmountOfPinsFails() {
$this->game->roll(-1);
}
/**
* @expectedException InvalidArgumentException
*/
public function testRollingMoreThan10PinsFails() {
$this->game->roll(11);
}
}
?>
This is one of many canonical examples used for teaching how to use an xUnit framework. It is not necessarily a comment on how to properly test or how to write testable code. It is an example to show that you can potentially use all normal language constructs in a test.
The failed sniffs were mainly due to the private methods.
phpcs --standard=PHPUnitStandard TestExample.php
52 | ERROR | Private methods are prohibited; found rollSpare
57 | ERROR | Private methods are prohibited; found rollMany
57 | ERROR | Function's cyclomatic complexity (2) exceeds allowed maximum of
| | 1
57 | ERROR | Function's nesting level (1) exceeds allowed maximum of 0
You can refer back to what I said in a a previous article to guess what my opinion is.
One possible solution is that you could copy and paste the internals of the private functions into the tests that call them. Most times private functions are small and not used in as many places in the test as you think. You may have heard that putting the couple lines into a well named private function increases readability, but it rarely does. It leads to jumping up and down in the code file, and most times it is more readable to see the code in a single block. The test shouldn’t be so long that you cannot see it in a single block.
BTW: A spare isn’t always two 5′s. So the private method doesn’t really describe the whole picture anyways.
This test however is pointing to a couple code smells.
First we will pick on rollSpare(). rollSpare() is likely a function that should exist in Game. It is very likely that you will want to rollSpare() when you really are playing a Game and not just when you are testing a Game. As I said in a previous article: Tests should always be treated like every other consumer of the subject under test.
NOTE: If it seems inappropriate to have the rollSpare() as part of the Game interface, then consider having some other class, maybe a helper, encapsulate this functionality within your production code base.
The second private function to pick on is rollMany(). This function adds logic which violates the cyclomatic complexity and nesting level constraints in addition to the no private methods constraint specified in the PHPUnit Unit Testing Code Standard. rollMany() probably cannot be argued to become part of the Game interface or even a helper in production code. This one could be a comment on your test. As in, you have redundant tests can lead to too many test.
It might sound silly that you could possibly test too much, but you can.
When writing unit tests you should write no more than one test per each equivalence class partition. It may sound harmless to do more than that, but you will be wasting precious time. You will waste time writing the extra tests. Your peers will waste time learning those tests. You and your peers will waste time maintaining those tests. All this waste will be for nothing, as the extra tests will not add extra coverage or safety. They are redundant.
rollMany() is an indicator that you have too many tests. You should only need the following tests to test roll()
- Single Roll
- Negative Pins
- More Than 10 Pins
- Zero Pins
- Ten Pins
- A Number of Pins that is greater than 0 and less than 10
- Multiple Rolls
Uh oh! We missed a couple tests! How are we going to test that we only bowl the ten frames with the correct number of rolls.
You could copy and paste the roll() call the number of times you need to roll the ball, and the number of time you need to roll the ball plus one. You would have to do this for a game with no spares, a game with all strikes, a game with one spare, a game with two spares, a game with three spares, a game with four spares, etc. and don’t forget that in the tenth frame you might need 3 balls! Oy!
Again, this is pointing to a problem with your production code. The interface for Game is too small, or maybe it is that we are missing an object in our model.
If we introduced a Frame, that had the roll() method, possibly like this
<?php
class Frame {
private $is_final_frame;
private $rolls;
public function __construct(
$is_final_frame = false
) {
$this->is_final_frame = $is_final_frame;
$this->rolls = array();
}
public function roll($number_of_pins) {
if (($number_of_pins < 0) || ($number_of_pins > 10)) {
throw new Exception(
"Must roll 0-10 pins, you rolled: $number_of_pins");
}
if (!$this->isComplete()) {
throw new Exception("Frame is complete");
}
$this->rolls[] = $number_of_pins;
}
public function isComplete() {
switch (count($this->rolls)) {
case 0:
return false;
case 1:
return $this->isStrike();
case 2:
return ($this->isStrike() || $this->isSpare())
&& !$this->is_final_frame;
default:
return true;
}
}
public function isStrike() {
return $this->rolls[0] === 10;
}
public function isSpare() {
return !$this->isStrike()
&& ($this->rolls[0] + $this->rolls[1]) === 10;
}
public function getRoll($roll_number) {
if ($roll_number >= count($this->rolls)) {
return 0;
}
return $this->rolls[$roll_number];
}
}
?>
…then we could do all our single roll tests and our frame completion tests in a sane manner with no control structures. Our tests would be
- Single Roll
- Negative Pins
- More Than 10 Pins
- Zero Pins
- Ten Pins
- A Number of Pins that is greater than 0 and less than 10
- Multiple Rolls – Not Final Frame
- One strike
- One strike, fail on second roll
- Two rolls, sum less than 10
- A spare
- Try to roll three balls
- Multiple Rolls – Final Frame
- One strike plus two more rolls
- A spare plus one more roll
- Fail on a strike or a spare followed by enough rolls to total four
- Two rolls, sum less than 10
- Fail on three rolls, when sum of first two rolls is less than 10
Finally, we can then have Game deal with Frames instead of roll()s, and can test that the game never goes beyond ten frames, or the number of frames it is given.
Game could look something like this
<?php
class Game {
private $frames;
private $current_frame_index;
public function __construct($frames) {
$this->frames = $frames;
$this->current_frame_index = 0;
}
public function roll($number_of_pins) {
if ($this->isComplete()) {
throw new Exception("Your game is complete");
}
$this->frames[$this->current_frame_index]
->roll($number_of_pins);
if ($this->frames[$this->current_frame_index]->isComplete()) {
$this->current_frame_index++;
}
}
public function isComplete() {
$max_frames = count($this->frames);
return ($this->current_frame_index >= $max_frames)
&& $this->frames[$max_frames - 1]->isComplete();
}
public function score() {
$score = 0;
$previous_was_strike = false;
$previous_was_spare = false;
foreach ($this->frames as $frame) {
if ($previous_was_strike) {
$score += 2 * ($frame->getRoll(0) + $frame->getRoll(1));
} else if ($previous_was_spare) {
$score += (2 * $frame->getRoll(0)) + $frame->getRoll(1));
} else {
$score += $frame->getRoll(0) + $frame->getRoll(1);
}
}
$last_frame = $this->frames[count($this->frames) - 1];
$score += $last_frame->getRoll(3);
return $score;
}
}
?>
I would recommend a Game_Factory for setting you up with the number of frames you want in the Game. Partly because it is bad practice to have logic or function calls in your constructor, but mostly because injecting complex objects or pre-populated arrays allows you to get your subject under test (SUT) to a testable state faster. (Note to self: I should probably write up a full example of that in a later article or workshop.) Not to mention our Game is much easier to vary now as we could swap in a different number of Frames or even a different variety of Framess. We could eventually extract a Scorer to handle scoring different Frame combinations. After all, even though this looks like a bowling game, it wasn’t called BowlingGame.
Anyways, I hope you are not distressed by now and think that this is the only solution. There are many ways to code a solution, but hopefully this will help you think about how to look at your tests as a reflection on your production code.
PHPUnit Coding Standard: Control Structures
Finally, on to Control Structures.
This should be short and simple.
In a unit test there should be absolutely no control structures.
Unit tests are meant to minimize your debug time by being small to the point tests on small specific pieces of code.
Typically there will be one Test_Case class per production class which will contain at least one test function per equivalence class partition of parameters for each function.
That may have been a bit much to parse, please read about equivalence class partitioning. They will help you from writing redundant tests
Since unit tests are intended to reduce the time spent debugging, it should make sense that the unit tests themselves should not be subject to debugging.
All logic should be tested. Adding control structures to tests is logic that should to be tested, and things that should be tested are possibly things that you may end up having to debug.
To warn about control structures, the coding standard checks for cyclomatic complexity of 1 and nesting level of 0. Cyclomatic complexity of 1 implies that there is one path through the test which also implies that the reader of your test can read the test from top to bottom and not have to loop or branch to understand it. Nesting level of 0 is just an extra precaution.
Please read the previous articles for some discussion on how to test without control structures.
Applicable Sniffs
Generic.Metrics.CyclomaticComplexity
Generic.Metrics.NestingLevel
PHPUnit Coding Standard: Functions
Now to talk about Functions. This could arguably be more controversial than the Naming and Classes standards.
There will be times where you raise your hand and cry out, “but that violates DRY principles!” or “the xUnit pattern book says this!” I assure you, I will provide further argument in later articles as none of these ideas exist in a vacuum.
Now let’s get to it!
No private methods in test files
Most of you will find yourself doing private methods to make your tests DRY (Don’t Repeat Yourself), but just remember that when things get too dry, they become brittle and break.
Sometimes it is more prudent to copy/paste that line or two into the test method itself. Honestly, it is a lot simpler to read the test in a single block than it is to jump up and down in the test file to track down what is hiding in a private method.
private methods are also cesspools for growing logic. If you read the previous articles about testing, you should know that tests are no place for logic, and that all logic should be considered part of the test framework or production code so that it is tested before use.
Let’s learn from past uses of private methods in unit test cases. Typically, you are either creating a custom assert, a custom setup, or you are encoding business logic.
In the case of a custom assert, please pull it out into the testing framework, or rather as an extension to the framework. I’ll write more about the best ways to do this later, but for now, just pull the asserts out into a class that lives with code that needs to be tested. You can also look at the Etsy PHPUnit Extensions wiki on Asserts and Constraints. Trust me, you will find that others might later need to use your custom assertions for new tests.
In the case of custom setup, we need to look at the problem a little harder. First, could you have used the inherited setUp() function to implement the set up. If yes, then please use that instead. If the answer is no because you have some instances of the SUT (subject under test) that need to be set up one way and some another, then you need to go back to an earlier consideration: can you just copy/paste. If copy/paste is a no go, and you couldn’t even offload some of the set up cost to setUp, then there is a problem with the construction of your SUT. You need to fix your production code because it is far too complicated to obtain an instance of this object. Maybe you can refactor your constructor into something simpler. You might have primitive envy, and could encapsulate commonly used together variables into objects. Another option is, you just might need a factory. While going through the exercise of creating a factory, you should learn more about the patterns for constructing your object. In your test, you can now use the factory to create your object for you, since it is a nicely tested piece of code. Maybe even some of your tests will be moved over to testing the object creation in the factory instead of cluttering the testing of the behaviors of a created object.
In the case where you are encoding business logic, you should seriously ask yourself why this business logic needs to be reconstructed in a test. There should be something in your production code that encapsulates the business logic.
Really, you are trying to test business logic. You don’t want to copy paste it in your test so you put that business logic in a private method, in your test code.
Ok, Mr. Fancy Pants, how is that business logic going to be implemented in production? If you have a method somewhere in your production code that does that business logic, then call that in the test and not the private method that copy/paste-d into the test code. If that business logic is hiding in a private method in your production, maybe you are being overzealous with protection levels. If that business logic is no where to be found in your production code, then why are you testing it. If that business logic is copy/paste-d all around your code base, then why was your test case so much more special than your production code.
It may seem mean taking out the usage of private methods from unit tests, but hopefully you now better understand how the presence of a private method in test code indicates a smell.
Applicable Sniffs
PHPUnitStandard.Testing.NoPrivateMethods
protected methods are reserved for overriding parent methods
If you look though the Test_Case implementations in PHPUnit, you should notice that the methods intended to be overridden for the most part are marked protected. This include setUp and tearDown.
The test case will run just fine if you override it with a public modifier, but it is an indicator to the reader of the test that you are overriding parent behavior. Also, the next criteria that I will present will not allow for methods that are not tests or providers to be marked public.
Just to double-check that you are not lost, these methods cannot be private either because that would definitely not override the function of higher visibility. Also, we do this as a whitelist check so that you do not squeak around the NoPrivateMethods sniff by upgrading to protected.
Applicable Sniffs
PHPUnitStandard.Testing.AllowedFunctionOverride
Only provider and test methods are allowed to be public
By provider, I mean a method you pass to @dataProvider in the phpdoc to provide data to a test method. See: PHPUnit DataProviders
By test, I mean a method that encapsulates a single test in a test case. The methods that begin with test or are annotated in the phpdoc with @test are test methods.
Both of these function types must be public for PHPUnit to see them. When PHPUnit cannot see a provider method, it will error out, but when PHPUnit cannot see a test function, it will silently not execute it.
Tests that are not executed will atrophy and die, but worse, they will confuse those who read the test. Most people will not notice that the modifier on a test function is not public, and they will read the test and assume that it has been executing and is valid. Readers of your test will then try to complete work based on these faulty assumptions and waste their time.
If you intend for you test not to execute then please mark it skipped, incomplete, or delete it.
Never be afraid to delete code when there is version control. It’s one of the many great reasons for version control.
Applicable Sniffs
PHPUnitStandard.Testing.TestOrProviderFunctionsOnly
PHPUnitStandard.Testing.TestOrProviderIsPublic
The final piece of the coding standard, as presented here, is Control Structures. This probably needs no separate article beyond this and the previous two. You should know by now that there should never be logic in you tests.
PHPUnit Coding Standard: Classes
Sorry for the delay, on to Classes
One class per test file
PHP allows for multiple classes per file, but for readability and better integration with your version control system, it’s just generally a good idea to have a single class per file, so why not do the same with your test files as well.
Given that you have one class in your production code file, then you will only need one TestCase class in your test file.
You may be tempted to add a class to your test file because you need a helper, but helpers are usually thought to be necessary because your object is not easy enough to use.
Always stop and look at the code under test before writing a test helper or PHPUnit framework extension. Chances are you are violating some object-oriented programming (OOP) aspect, and you are being unfair to others, who will later touch your code, by adding a test helper as a kludge.
If you have to write a test helper, then you should still resist adding it to the test file. Test helpers, justified or not, usually grow logic, and if it doesn’t have logic yet, it will exist as a bed for growing logic later.
Anything that can grow logic, will grow logic!
All logic should be tested, otherwise it will be just that much more code to debug when something goes wrong. So if you really need a test helper, either add it to another code library or include it with your production code.
Including test helpers with your production code might seem silly at first, but after enough experience you will see just how often that those test helpers will become production helpers.
You may also be tempted to add a class to your test file because you want to manually mock one or more of your objects collaborators or you want to override some functionality from your class under test.
There are plenty of great mocking frameworks. Please use them. Most of the mocking frameworks have great documentation, and if that is not enough to help you learn how to use mocking framework, please reach out to one of the contributors. Open source contributors want you to use their projects, and hearing from those who are lost will only make the projects more intuitive, easier to use, and better documented.
Using mocking frameworks will help with eliminating extra classes in test files that exist because of manually mocking collaborators. So what about overriding some functionality in the class under test? STOP! Listen to yourself! What are you testing if you are overriding part of the object? Do you know what encapsulation means?
Please also see:
Applicable Sniffs
PHPUnitStandard.Testing.ExtraneousClass
Name should match file name (with packaging)
Since we have only one class per test file, why not have the names match?
Aside from keeping things pretty, PHP is typically not happy when more than one class has the same name. Since we have only one class per test file, and you will only be able to save one file with that name in the directory (package) anyway, might as well take advantage of that nomenclature in naming your class as well so you can stop wasting time running most of the PHPUnit test suite before discovering you have a couple classes with the same name.
Applicable Sniffs
PHPUnitStandard.Testing.ClassName
Extend only tested TestCase classes
Please see previous comments about anything that can grow logic will grow logic and having your test helpers in your production code or external library and not in your test code.
Unit tests should help you debug production code, not be included in the code that needs to be debugged.
The TestCase class that your test class extends is part of your test, it is a test helper, and it should be tested. It is possibly even more important to make sure your TestCase base class is tested because you likely have several test classes based on it. If the basis of so many tests in your suite is faulty, what does that say about the relevancy and effectiveness of your test suite.
Applicable Sniffs
PHPUnitStandard.Testing.ProvenTestCase
No interfaces or abstract classes
We have already covered that each test file is going to contain only one class, which is a test class for the class under test with matching name, and that the class will extend a tested TestCase. There will be no test helpers in the test files, so there is absolutely no place in the testing directory for interfaces and abstract classes to live. Simple.
Applicable Sniffs
PHPUnitStandard.Testing.NoInterfaces
Next up, Functions
PHPUnit Coding Standard: Naming
I promised more details about the PHPUnit coding standard, so let’s start with Naming.
Pick a directory just for tests
Having a directory just for tests is a communication point. It communicates that only tests live here. There is no production code here. There is also no reason to ever think that the code in this directory needs to be looked at in addition to production code when debugging an issue, at least, if you follow this PHPUnit Coding Standard.
Applicable Sniffs
PHPUnitStandard.Testing.NoInterfaces
PHPUnitStandard.Testing.ProvenTestCase
Pick a test file name suffix, ie. Test.php
The default test file name suffix in PHPUnit is Test.php.
When you execute
phpunit .
the test runner recursively searches through the current directory for all files ending in Test.php. That means all files without that suffix will be ignored, and you could be inadvertantly skipping a lot of tests.
If for some reason you decide you must have a different suffix, say Tests.php, then you will have specify in a PHPUnit XML config file like so
<testsuites>
<testsuite name="My Test Suite">
<directory suffix="Tests.php">.</directory>
</testsuite>
</testsuite>
Alternatively, you could list each test file, but then you are likely to forget to add new tests to the configuration.
Tests that are not executed regularly are dangerous. Tests that are not executed regularly tend to atrophy. New developers will look at tests has a guideline for how to use production code. If a test is not regularly executed then it is out of date documentation that someone will take as gospel, and essentially, they will be wasting their time.
Applicable Sniffs
PHPUnitStandard.Testing.FileNameSuffix
Match the name of the test file to the name of the file being tested
If all the tests are in a test directory, then we know where to find all the tests.
If all the tests are named in a way that matches the naming of the code under test, then we know where the unit test for a production class file is. We can then quickly answer the questions of
- Do I have a test for this code?
- If I do have a test for this code, how can I use this code?
Applicable Sniffs
PHPUnitStandard.Testing.ClassName
Next time we’ll discuss Classes
PHPNW11
Slides for my talk “Are Your Tests Really Helping You?” are now available on Slide Share.
As presented at PHPNW11 in Manchester, UK.
PHPUnit Coding Standard: https://github.com/elblinkin/PHPUnit-CodeSniffer
(P.S. I owe tons of documentation and a few “removal of assumptions”)
BTW: #PHPNW11ROCKS
Preview: A Unit Testing Standard
As I mentioned in the last blog post, , I have been playing with PHP_CodeSniffer, and I have devised a standard for assessing the cleanliness and efficiency of the PHPUnit tests that are supposed to be unit tests.
If your unit tests pass this standard, then your tests should have taken less time to write, will take less time to debug, and there will be less time spent maintaining because everything in your tests would be perfectly straight-forward and pretty much prove that your interfaces for the code under test are simple to use as there would be no funny business need to prep those objects in your test code. This standard will be part of the tool set I will be presenting at PHPNW, so I will begin unveiling it.
Currently, there are about a dozen things in the standard to sniff for in your unit test code:
- Filename Suffix Must Be Test.php
- File Must Contain Class with Matching Name
- Test and Provider Functions Must Be Public
- Unused Data Providers
- Class Must Extend Trusted Test Case
- File May Contain Only One Class
- No Interfaces
- No Private Methods
- Only Allowed Function Overrides
- Public Functions Must Be A Test or Provider
- Cyclomatic Complexity of 1
- Nesting Level of 0
Some of these could probably be used on non-unit tests, but since unit testing tends to be the simplest, cheapest, and most scalable with a growing workforce and codebase, let’s stick with talking about unit tests.
