Hi Stevan,

I am going to be a bit more picky this time around, but please do not take it too bad.

> === modified file 'README'
...
> +Unit tests
> +----------

I don't think it's that interesting that these are unit-tests.  FWIW, we can perfectly well have Python unit tests in this same branch, so this is a wrong title in my opinion.

Relevant title would be "Tests for PHP license-matching logic" or similar.

You should also make this a subsection of "Tests" section below it.  This README file is using ReST, so all it takes to get a "subsection" is to use a different underlying character (the file already uses "." above).

> +
> +There's currently only one unit test file, LicenseHelperTest.php under testing directory. You first need to install the phpunit package from ubuntu repos:

It would be good to follow the 79-character limit in the README as well.  It was probably me who messed this up originally, so let's not worry about it right now.

> +$ sudo apt-get install php-pear
> +$ sudo pear config-set auto_discover 1
> +$ sudo pear install pear.phpunit.de/PHPUnit

Generic instructions should be "sudo apt-get install phpunit".  The fact that phpunit package is broken in current Ubuntu/Debian releases is not something we want to rely on.  FWIW, Ubuntu 10.04 (previous LTS, which most of our servers still run and will run for another 6 months) has a working phpunit package (it doesn't seem to have assertNotEmpty method, though).

> +
> +Then to run the test from the 'testing' directory:
> +$ cd testing/
> +$ phpunit LicenseHelperTest
> +
> +In case the tests fail with PHP_CodeCoverage error, do the following to install the phpunit properly:
> +
> +$ sudo apt-get remove phpunit
> +$ sudo apt-get upgrade pear
> +$ sudo pear channel-discover pear.phpunit.de
> +$ sudo pear channel-discover pear.symfony-project.com
> +$ sudo pear channel-discover components.ez.no
> +$ sudo pear update-channels
> +$ sudo pear upgrade-all
> +$ sudo pear install --alldeps phpunit/PHPUnit
> +$ sudo apt-get install phpunit

Your former instructions solve that problem as well.  You should list either one or the other, not both.  You should also mention what we know of the problem (i.e. "recent (as of 2012-05-08) Ubuntu/Debian releases have a broken phpunit package and thus require installation in a different manner").

Also, please use some indentation for commands like it's already done in the 'Tests' section for testr ones.


> === added file 'licenses/LicenseHelper.php'
> --- licenses/LicenseHelper.php	1970-01-01 00:00:00 +0000
> +++ licenses/LicenseHelper.php	2012-05-07 15:13:20 +0000
> @@ -0,0 +1,104 @@
> +<?php
> +
> +class LicenseHelper 

Trailing whitespace, please remove it.  If you are using Emacs (I believe you are), you can have it highlighted by adding the following to your .emacs:

  (setq show-trailing-whitespace t)

Note, I understand you have mostly copied the code over in this file, so that makes it harder for me to comment on it.  Thus, I'll focus on style first.

> +{
> +
> +  // Get list of files into array to process them later.
> +  // Used to find special licenses and dirs with only subdirs.
> +  public static function checkFile($fn)
> +  {
> +    if (is_file($fn) or is_link($fn)) {
> +      return true;
> +    }
> +    return false;
> +  }

Please use four-space indentation in this file, or at least follow the existing (but otherwise bad) tab-indentation style: we want the entire project to use similar style for all of code, and Python code is already using 4-spaces.

I see there is also http://pear.php.net/manual/en/standards.php which seems to be what PEP8 is for Python.  We should start a discussion on whether that's what we want to follow and how should we adapt it to suit our needs and cross-language nature.

In particular, http://pear.php.net/manual/en/standards.indenting.php agrees with what I am proposing here.

> +
> +  public static function getFilesList($dirname)
> +  {
> +    if (!is_dir($dirname)) {
> +      throw new InvalidArgumentException('Method argument should be a directory path');
> +    }

79-character line limit, I know it wasn't respected originally, please fix it here.

> +
> +    $files = array(); 

Another trailing whitespace.

> +    if ($handle = opendir($dirname)) {
> +      while ($handle && false !== ($entry = readdir($handle))) {
> +	if ($entry != "." && $entry != ".." && !is_dir($dirname."/".$entry) && $entry != "HEADER.html") {

More overflow.

...
> +
> +  // Get array of file name and extension from full filename.
> +  public static function splitFilename($filename)
> +  {
> +    $pos = strpos($filename, '.');
> +    if ($pos === false) { // dot is not found in the filename
> +      return array($filename, ''); // no extension
> +    } else {
> +      $basename = substr($filename, 0, $pos);
> +      $extension = substr($filename, $pos+1);
> +      return array($basename, $extension);
> +    }
> +  }

I thought we agreed to get rid of this function?

> +  {
> +    if (!empty($fl)) {
> +      foreach ($fl as $f) {
> +	if (preg_match($pattern, $f, $matches)) {
> +	  return $f;
> +	}

Indentation here seems entirely broken.

> === modified file 'licenses/license.php'

Changes here look good.


> === added file 'testing/LicenseHelperTest.php'

For this new file, please apply the coding style as suggested above: 4 space indentation, 79 character line limit, no trailing whitespaces.

> --- testing/LicenseHelperTest.php	1970-01-01 00:00:00 +0000
> +++ testing/LicenseHelperTest.php	2012-05-07 15:13:20 +0000
> @@ -0,0 +1,122 @@
> +<?php
> +
> +class LicenseHelperTest extends PHPUnit_Framework_TestCase
> +{
> +
> +  /**
> +   * Includes test class, creates some help files for testing
> +   */

These are comment styles I like to see (/**/ instead of //) on method names. // are for inline comments, imho.

Please make the narrative active instead of passive ("Include the class to test and create helper files to use while testing." or similar).  Finish sentences with punctuation :)

> +  protected function setUp()
> +  {
> +    require_once("../licenses/LicenseHelper.php");
> +    symlink(__FILE__, "test_link");

This sounds a bit scary: what if somebody accidentally did some writing to the test file in one of the tests, without looking at this set-up method?  Is it really that costly to create a new temporary file instead?  I'd just use tempnam() to create one.  I'd also assign it to a property of the instance so it's easily available from tests that need to use it (and from tearDown), so it wouldn't have to be hard-coded in multiple places.

Also, you are using this only in a single test, yet it's created and destroyed with every test in this test class: that's a big waste.  Let's move it to that one test we do need it in, wrap it in a try..finally block (hopefully PHP has that, if not, in a generic exception catcher :)

> +  }
> +
> +  /**
> +   * Removes help files

"Remove helper files used in testing."

> +   */
> +  protected function tearDown() 

Trailing whitespace :)

> +  {
> +    unlink("test_link");
> +  }
> +
> +  /**
> +   * Test with directory

"Running checkFile on a directory path returns false."

> +   */
> +  public function testCheckFile_nonFile()

Please name all tests for methods as test_methodName_something() instead of testMethodName_something.

> +  {
> +    $this->assertFalse(LicenseHelper::checkFile(dirname(__FILE__)));
> +  }
> +
> +  /**
> +   * Test with link

"Running checkFile on a symbolic link to an existing file returns true."

Which reminds me, what happens if it's run on a symbolic link to a non-existant file?

> +   */
> +  public function testCheckFile_link()
> +  {
> +    $this->assertTrue(LicenseHelper::checkFile("test_link"));
> +  }
> +
> +  /**
> +   * Test with file

"Running checkFile on a regular file returns true."

> +   */
> +  public function testCheckFile_file()
> +  {
> +    $this->assertTrue(LicenseHelper::checkFile(__FILE__));
> +  }
> +
> +  /**
> +   * Get file list from a file

"getFileList throws an InvalidArgumentException when passed an argument pointing to a file."

> +   */
> +  public function testGetFilesList_file()
> +  {
> +    try {
> +      $file_list = LicenseHelper::getFilesList(__FILE__);
> +      $this->assertTrue(FALSE);
> +    } catch (InvalidArgumentException $e) {
> +      $this->assertTrue(TRUE);
> +    } catch (Exception $e) {
> +      $this->assertTrue(FALSE);
> +    }
> +  }

Any reason you are not using the following approach here:

http://www.phpunit.de/manual/3.2/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.exceptions.examples.ExceptionTest.php


> +
> +  /**
> +   * Get file list from a directory

"getFileList returns a list of filenames in that directory."

> +   */
> +  public function testGetFilesList_dir()
> +  {
> +    $file_list = LicenseHelper::getFilesList(dirname(__FILE__));
> +    $this->assertNotEmpty($file_list);
> +    $this->assertContains(basename(__FILE__), $file_list);
> +    
> +    // Remove '.' and '..'.
> +    $expected_count = count(scandir(dirname(__FILE__))) - 2;
> +    $this->assertCount($expected_count, $file_list);
> +  }

I dislike this test:  if there was a bug in getFilesList so that it read a different directory than the one passed in, it might still accidentally succeed (if the one it worked on had the same number of files in it).  We should construct all our test data on-the-fly.  I don't know how's that best done with PHP, but in Python, I'd do the following:

 - create a temporary directory and store the path
 - put a few files I want in
 - assert that getFilesList() returns exactly the files I put in
 - remove the temporary directory in a "finally" block

> +
> +  /**
> +   * Test with pattern which will not match any filename.

I'll let you come up with better narrative for further test cases in the style I suggested.

> +   */
> +  public function testFindFileByPattern_noMatch()
> +  {
> +    $file_list = array("test.txt", "new_file.pdf");
> +    $pattern = "/^abc/";
> +    $this->assertFalse(LicenseHelper::findFileByPattern($file_list, $pattern));
> +  }
> +
> +  /**
> +   * Test with pattern which will match a filename.
> +   */
> +  public function testFindFileByPattern_match()
> +  {
> +    $file_list = array("test.txt", "new_file.pdf");
> +    $pattern = "/test/";
> +    $this->assertEquals("test.txt", 

Trailing whitespace.

> +			LicenseHelper::findFileByPattern($file_list, $pattern));

And bad indentation (tabs).  To span multiple lines, you can also do the following:

    $this->assertEquals(
        "test.txt", LicenseHelper::findFileByPattern($file_list, $pattern));

> +  }
> +
> +  /**
> +   * Test with no eula present.

"getTheme returns a generic Linaro-branded template when no EULA is present."

> +   */
> +  public function testGetTheme_noEula()
> +  {
> +    $eula = "EULA.txt";
> +    $filename = "snowball.build.tar.bz2";
> +    $this->assertEquals("ste", LicenseHelper::getTheme($eula, $filename));
> +    $filename = "origen.build.tar.bz2";
> +    $this->assertEquals("samsung", LicenseHelper::getTheme($eula, $filename));
> +    $filename = "build.tar.bz2";
> +    $this->assertEquals("linaro", LicenseHelper::getTheme($eula, $filename));
> +  }

This test doesn't match the description (or at least what I expected in it).  It should also be split into at least 3 separate tests: one for snowball, one for origen, and another generic one.

> +
> +  /**
> +   * Test with eula present.
> +   */
> +  public function testGetTheme_eula()
> +  {
> +    $eula = "EULA.txt.test";
> +    $this->assertEquals("test", LicenseHelper::getTheme($eula, ""));

I am not sure what this test demonstrates either.  Please improve the comment so readers don't have to go back to code to understand what's going on here.


Finally, don't be stumped with the amount of suggestions I am making.  They are mostly cosmetic, but that's important in projects that multiple people are going to be touching on and maintaining over a long period of time.  It makes adjustments from one project to the other easier, and allows us to be more effective.

I am generally very happy with your work, so just keep it up.  And let's get this fixed up and then landed.

  review needs-fixing

Cheers,
Danilo