Code coverage analysis

[globalcitizen]

Figure out a way to make code coverage analysis work with our built-in, non phpunit based testing approach. At the time of writing, I'm unaware of an easy solution here.

[acoulton]

Would you instead consider having the tests ported to phpunit? That might be the easiest solution for code coverage and would add some other benefits (eg failing the test on php errors which I think at present aren't caught by your test routines). I could make a PR for that pretty quickly if you'd be interested.

[globalcitizen]

Well, frankly I'd very strongly prefer a real solution as I hate writing and maintaining OO PHP code... it causes me mental exceptions. Of course, PHP errors do fail the build right now, same as an individual test throwing an error.

[acoulton]

@globalcitizen I don't think that's correct - PHP errors are output to the console, but do not fail the build (apart from fatal errors, obviously).

As a quick test locally I added $foo = $bar to the top of the verify_iban function and ran php utils/other-tests.php. The test output was:

Other tests:
 - verify_iban() test #0... PHP Notice:  Undefined variable: bar in /vagrant/php-iban/php-iban.php on line 13
PHP Stack trace:
PHP   1. {main}() /vagrant/php-iban/utils/other-tests.php:0
PHP   2. verify_iban() /vagrant/php-iban/utils/other-tests.php:28

Notice: Undefined variable: bar in /vagrant/php-iban/php-iban.php on line 13

Call Stack:
    0.0005     237272   1. {main}() /vagrant/php-iban/utils/other-tests.php:0
    0.0046     694792   2. verify_iban() /vagrant/php-iban/utils/other-tests.php:28

OK.
{output of other tests removed for conciseness}
All tests passed.

and the script exited with status code 0.

Obviously you could resolve this with a custom error handler, but you get it for free with phpunit as well as other benefits including for example seeing the results of all the assertions in your test run rather than just failing and exiting on the first failed test.

[acoulton]

I appreciate your concerns about OO PHP code, but I don't think a PHPUnit test case is really that complex in terms of object orientation. For example, consider this part of "other-tests.php":

# === verify_iban machine_format_only mode ===============================
$test_data = array(
                  # input                   # machine_format_only?  # expected
            array('GB29 NWBK 6016 1331 9268 19',        true,           false),     # spaces present, machine_format_only mode
            array('GB29 NWBK 6016 1331 9268 19',        false,          true),      # spaces present, normal (relaxed) mode
            array('IBAN GB29-NWBK-6016-1331-9268 19',   true,           false),     # spaces + prefix + dashes, machine_format_only
            array('IBAN GB29-NWBK-6016-1331-9268 19',   false,          true),      # spaces + prefix + dashes, normal mode
            array('IIBAN GB29-NWBK-6016-1331-9268 19',  false,          true),      # spaces + prefix + dashes, normal mode
             );
$i=0;
foreach($test_data as $this_test) {
 print " - verify_iban() test #$i... ";

 if(verify_iban($this_test[0],$this_test[1]) !== $this_test[2]) {
  print "FAILED.\n";
  exit(1);
 }
 else {
  print "OK.\n";
 }
 $i++;
}

Expressed as a PHPUnit testcase it could look something like:

class OtherTest extends \PHPUnit_Framework_Testcase {

  /**
   * @testWith ["GB29 NWBK 6016 1331 9268 19",       true,  false, "spaces present, machine_format_only mode"]
   *           ["GB29 NWBK 6016 1331 9268 19",       false, true,  "spaces present, normal (relaxed) mode"]
   *           ["IBAN GB29-NWBK-6016-1331-9268 19",  true,  false, "spaces + prefix + dashes, machine_format_only"]
   *           ["IBAN GB29-NWBK-6016-1331-9268 19",  false, true,  "spaces + prefix + dashes, normal mode"]
   *           ["IIBAN GB29-NWBK-6016-1331-9268 19", false, true,  "spaces + prefix + dashes, normal mode"]
   */
  public function test_verify_iban_machine_format_only_mode($input, $machine_format_only, $expected)
  {
    $this->assertSame($expected, verify_iban($input, $machine_format_only);
  }
}

Aside from the call to $this->assertSame, really all that's changed is the header/footer around the assertion - and arguably I'd say that as it's a lot less verbose the intent and behaviour of the test is a lot clearer.

And supposing the behaviour of machine format broke, with your suite you'd perhaps get:

Other tests:
 - verify_iban() test #0... FAILED

Where PHPunit would give more like:

F.F..
There were 2 failures:

1) OtherTest::test_verify_iban_machine_format_only_mode with data set #0 ("GB29 NWBK 6016 1331 9268 19", true, false, "spaces present, machine_format_only mode")
Failed asserting that true matches expected false

2) OtherTest::test_verify_iban_machine_format_only_mode with data set #2 ("IBAN GB29-NWBK-6016-1331-9268 19", true, false, "spaces + prefix + dashes, machine_format_only")
Failed asserting that true matches expected false

As well as the result of all the other tests in the suite.

Up to you, obviously, but I really do think it would be worth considering and would be happy to do the initial port if you like.

[globalcitizen]

Thanks for your response.

It seems you are confused about the difference between an error and a warning.

To exit with an error code on all warnings, it's only necessary to run in strict mode which can be achieved with code or by executing php from .travis.yml with the appropriate arguments.

There is no practical usefulness in the current development workflow to continue running tests if one fails, if it were required it would be a trivial change.

Notice also how you provide an example based upon the OO PHP wrapper, but do not provide an example based upon the core (functional) code. This is probably (I suspect) because phpunit has code smells whereby it has optimized specifically for an OO usecase and is impractical to apply painlessly to functional code, thus subtly encouraging needless complexity, a terrible property of most OO codebases. In the functional codeblock, the programmer can comprehend precisely the execution flow purely from looking at the code. In the OO phpunit-based codeblock, the programmer must look further to documentation and classes for phpunit, notably how to launch text execution, and the behavior of PHPUnit_Framework_Testcase, the various available assert functions, their arguments, etc. The longer you program, the more you appreciate simplicity.

Joe Armstrong put it this way: "The problem with object-oriented languages is they've got all this implicit environment that they carry around with them. You wanted a banana but what you got was a gorilla holding the banana and the entire jungle."

(I'm not against OO per-se, in fact I really enjoy it sometimes, for example the power of introspection in languages such as ruby, but the adoption of a well-tested OO-paradigm test library is just not adding enough concrete benefit here versus this library's needs (not very complex at present) and the fact that we have an already working approach with no dependencies.)

Weighing up the pros and cons, I am not interested in adding a dependency and rewriting functional code for the vague potential benefit of code coverage analysis. Sorry. However, it should be possible to add OO PHP tests in a non-destructive way, and if you would like to duplicate (versus rewrite) and maintain them in the manner you describe to the existing OO PHP wrapper this would have the side benefit of testing the code coverage of the OO PHP wrapper.... and a pull request will be accepted.

[gaetan-be]

+1 on porting the tests to PHPUnit. I'm sure you can find a way of running your tests like you do now and having the same tests running in PHPUnit without too much rewrite (or porting).

[globalcitizen]

Well, be my guest. As I said, I'm not going to maintain them.

[acoulton]

@globalcitizen I think we'll probably have to agree to disagree, but just to pick up a couple of points from your response:

It seems you are confused about the difference between an error and a warning

Nope, but perhaps my language could have been more precise. I was using "php error" in the general sense (consistent with the collective naming PHP itself employs - errors, warnings, notices etc are of all levels are documented together in the Error Handling and Logging section of the manual and handled with functions like error_reporting(), error_get_last().

IMO, a package that is intended for use as a single component in a bigger system should not emit php errors of any kind (including warnings and notices) unless that behaviour is explicitly documented. Therefore the presence of PHP notices etc should fail the build in this context.

There is no practical usefulness in the current development workflow to continue running tests if one fails, if it were required it would be a trivial change.

Perhaps for you working alone - in my experience as contributor and maintainer of community projects it is much easier for people new to the project and for maintainers if you can see (eg on a PR build) the whole picture of what is/is not broken together with a clear diff between the expected and actual result.

Notice also how you provide an example based upon the OO PHP wrapper, but do not provide an example based upon the core (functional) code.

Incorrect. The example I provided was a direct port of your functional test for the functional wrapper. The key point (in fact basically the only line of code in my example) being the call to the global function verify_iban() in $this->assertSame($expected, verify_iban($input, $machine_format_only));

Of course the test case itself is structured as an object, as PHPUnit is by default and at its most powerful when using object-structured test cases, but this has no bearing on the architecture of the code under test.

In the OO phpunit-based codeblock, the programmer must look further to documentation and classes for phpunit, notably how to launch text execution, and the behavior of PHPUnit_Framework_Testcase, the various available assert functions, their arguments, etc.

Perhaps. Though I'd counter:

• Nothing to stop you putting a test.sh executable in the root of the project for people who aren't sure how to run phpunit. Though I'd question if that's really harder than figuring out how to run your own tests. Normally if I'm new to a project I just grab the command from .travis.yml
• Behaviour of the testcase is a fair point, though I'd think once you have an existing one it's fairly obvious you can add a further test_something method to add another test
• Nothing forces you to find out about all the other assertions. You could quite happily code everything with $this->assertSame() if you didn't care to look for eg $this->assertTrue(). And of course if you use an IDE those assertions may be completed for you.

More importantly IMO is that phpunit is the de-facto standard and there are very many people already familiar with the answers to all the points you raise. For those that aren't, I don't think there's a hugely bigger barrier to entry than there is to follow the flow of your custom test structure and that effort will then be reusable on other projects.

The existence of the implicit environment also means that the visual and logical focus of the testcase code is very strictly on exactly what your library is actually supposed to do in various conditions.

The longer you program, the more you appreciate simplicity.

I don't think this is about time served (I've been programming some 20 years), but about what you find simple and in particular what your experience to date has taught you speeds/slows things up.

I appreciate the question of "what you find simple" is a personal one. I genuinely find your tests more complex than the alternative, but there's no point me pushing you to switch to something you don't want.

However, I don't see any point whatsoever in duplicating the current tests in PHPUnit - I can't see how that would add any value, and it would introduce significant brittleness and/or potential for things to get missed by duplicating specifications into two places. The longer you program, the more you appreciate avoiding duplication....

[globalcitizen]

IMO, a package that is intended for use as a single component in a bigger system should not emit php errors of any kind (including warnings and notices) unless that behaviour is explicitly documented. Therefore the presence of PHP notices etc should fail the build in this context.

Sounds logical. Feel free to submit a PR or open an issue. Here is not the place.

The existence of the implicit environment also means that the visual and logical focus of the testcase code is very strictly on exactly what your library is actually supposed to do in various conditions.

The term implicit basically sums it up for me, from an objective standpoint. The rest is subjective, and down to personal taste.

The longer you program, the more you appreciate avoiding duplication....

Someone should mention that to the numerous ripoff libraries ...

[christophermichaelthomasmillar]

Sorry for the Necro Post but I thought that I would mention PEST. https://pestphp.com/docs/writing-tests

I don't want to get into the OO vs. FP debate but for those who are vehemently opposed to writing OOP test cases using PHPUnit, PEST might be a good fit. PEST is a clone of JEST written on top of PHPUnit, I don't think there is anything you can't do with it and you don't need to write tests in an OOP way.

[globalcitizen]

Situation as before. Feel free to contribute revised tests if you are willing to maintain them.