Herman J. Radtke III

Read more of my blog or subscribe to my feed.


Patching a bug in a pecl extension

Written by Herman J. Radtke III on 08 May 2011

In my last post I explained how to build a development version of a pecl extension.  Now we will go through the bug lifecycle in the pecl/memcache extension.  Besides writing the actual C code to fix the bug, it is considered a best practice to write a test that verifies the bug has been fixed.  I will use PECL bug #16442 - memcache_set fail with integer value as an example, even though it is already been fixed. Whether creating a patch for a bug fix yourself or applying a users patch to an existing bug, the process is pretty much the same.  The very first thing I do is create a .phpt test for the bug.  A .phpt test is a functional test used in php core and many pecl extensions.  All bug fixes should include a simple test so we can verify the bug is indeed fixed by the patch and to prevent a regression in the code where the bug is reintroduced.  The .phpt functional tests are quick to write and have such a positive long term investment I rarely find myself not wanting to write them. Here is an example test: --TEST-- PECL bug #16442 (memcache_set fail with integer value) --FILE-- <?php include 'connect.inc'; memcache_set($memcache, 'test123112', 1, MEMCACHE_COMPRESSED, 30); $ret = memcache_get($memcache, 'test123112'); var_dump($ret); echo "Done\n"; ?> --EXPECT-- int(0) Done There are three basic parts to a .phpt file: test information, the test itself and the expected test results.  The test information section is denoted by the "--TEST--" heading.  This is where information about the test is placed.  For bug fixes I normally include the bug number and a brief description of the bug.  The brief description sometimes matches the bug title, but there are cases where the bug title doesn't describe the actual bug so I will make a better description up. After the informational block  is the actual test itself, denoted by the "--FILE--" heading.  The test itself is always normal php code.  You will notice that the test includes a 'connect.inc' file.  This is a standard convention for tests that depend on some outside system or environment.  In the case of pecl/memcache, the connect.inc sets up a number of connections to memcache using various ip addresses, ports and protocols (tcp and udp).  This is done to make sure all tests are testing against the same servers and to reduce the amount of boilerplate code in the tests.  It is considered a best practice to use var_dump() for any kind of test output.  This makes the output as explicit as possible.  You will also notice many tests end with an echo "OK\n" or an echo "Done\n" statement.  This is done to explicitly define the end of the test.  This can help catch extra test output, such as warnings or errors, that should cause the test to fail. The last section is the expected test results section.  There are a number of different ways to express test results, including normal string comparison (--EXPECT--), printf style formatting (--EXPECTF--) and regular expressions(--EXPECTREGEX--).  Explaining how each of them work and when to use each one is would take an entire blog article itself.  Luckily, the PHP QA team has an article that discussions each one in detail.  I almost always use EXPECTF for the test results, even if I am not using any format characters.  This is more the result of copy/paste than any explicit decision on my part.  You should try to be as generic in your test output as possible.  For example, if you are expecting a warning to show in the test results, make sure you do not explicitly check for your local path to the file in the warning output.  This will cause the test to fail for anyone else. I mentioned earlier that there are three parts to a .php test, but I lied.  There are actually a few more, but they are less commonly used.  I suggest you read through the PHP QA article on writing .phpt tests for a full explanation of .phpt test syntax and usage.  I am merely highlighting the major points to get us up and running. I have noticed that there is a general best practice to naming .phpt test files.  Non bug fix .phpt tests are sequentially numbers starting from 001.  I call these feature tests.  As development continues on a package, new tests are created.  You will notice there are over 100 features tests in pecl/memcache/branches/NON_BLOCKING_IO/tests.  For bug fixes, .phpt tests follow the format of 'pecl{bug #}.phpt'.  This means I would name the .phpt test file for PECL bug #16442 as pecl16442.phpt. Naturally we want to run the test once it is written.  The php community has provided a tool, called run-tests.php, that runs all or a subset of tests for a given package.  The run-tests.php script is pretty flexible.  You can specify a directory to run all tests in, specify a list of tests to run or specify a single test to run.  Here are some examples: # run a single test TEST_PHP_EXECUTABLE=/usr/local/php/bin/php php run-tests.php tests/pecl16442.phpt # run a two tests TEST_PHP_EXECUTABLE=/usr/local/php/bin/php php run-tests.php tests/pecl16442.phpt tests/pecl16536.phpt # run all tests in the tests/ directory TEST_PHP_EXECUTABLE=/usr/local/php/bin/php php run-tests.php tests/ The syntax of these commands might look a little strange.  The run-tests.php does not try to assume which php executable you want to run the tests against.  Without this variable being set, the script will throw an error that looks like: "ERROR: environment variable TEST_PHP_EXECUTABLE must be set to specify PHP executable!".  I am using a bash trick to set the PHP_TEST_EXECUTABLE variable before running each test.  You can also export this variable so it is available each time you run the tests.  If you decide to export it, you should consider adding it to your .bash_profile so you don't have to do it each time you login.  Whichever way you do it, as long as you set this variable before running the run-tests.php script you will be fine. Running the tests will generate some nicely formatted output.  The output is self-explanatory, so I will not go over it here.  I do want to talk about some interesting things that happen when a test fails.  I changed the "--EXPECT--" portion of my test example to "int(0)".  If you run this test you will notice it listed in the "FAILED TEST SUMMARY".  What you don't see is the actual test output or even more importantly why test failed.  Thankfully, run-tests.php has us covered.  If you look at the output of the tests/ directory you will notice some new files: tests/pecl16442.diff # a diff of the expected output and results tests/pecl16442.exp # the expected output tests/pecl16442.log # a log of the test run tests/pecl16442.out # the actual test output tests/pecl16442.php # the php code used to run the test I find the .out and .php files most useful when debugging code.  When trying to fix a bug I will run the test using run-tests.php once and the continually test against the .php file until I am reasonably sure I have fixed the bug.  I will then use run-tests.php again and see if it passed.  The .out file is useful to see exactly what the test result actually was.  I sometimes use the .diff file when I am head scratching as to why a test I think should have passed is not passing.  Some trivial issues such as newlines and extra spacing can sometimes create a false positive in an otherwise passing test. After applying the fix to the memcache source code to allow integer values in memcache_set and using run-tests.php to check if the test pecl16442.phpt passes, we need to make sure we run all the tests.  As I mentioned earlier, the existing tests are a there to help prevent regressions in the source code.  One should always check that all tests pass before committing any new code.  I will mention that this can be frustrating when working with a pecl extension that has tests that always seem to fail.  If this is the case, use your best judgement.  You may also want to fix the failing tests before doing any new work on the extension too. Once all the tests pass, commit your changed source code files and the .phpt test file(s).  Make sure you do not include any of the failed test files that are automatically generated.  If you do not have commit access, include the source code files in the bug report along with the test.  There is nothing any pecl maintainer likes more than having tests included with the patch.