Hi Stevan,

Thanks for working on this.  We are so close to getting this landed now.
Just a few more comments.

У уто, 08. 05 2012. у 16:11 +0000, Stevan Radaković пише:

> разлике међу датотекама прилог (review-diff.txt)
> === modified file 'README'
> --- README	2012-05-04 17:44:47 +0000
> +++ README	2012-05-08 14:30:26 +0000
> @@ -74,6 +74,8 @@
>  build steps, and not as publishing steps.
>  
> 
> +
> +

Heh, how many newlines is enough?  Please get rid of these two.

>  Tests
>  -----
>  
> @@ -82,3 +84,20 @@
>  
>    $ testr init
>    $ testr run
> +
> +
> +Tests for PHP license-matching logic
> +....................................
> +
> +There's currently only one unit test file, LicenseHelperTest.php under testing directory. You first need to install the phpunit package from ubuntu repos:
> +  $ sudo apt-get install php-unit
> +  
> +
> +Recent (as of 2012-05-08) Ubuntu/Debian releases have a broken phpunit package and thus require installation in a different manner:
> +  $ sudo apt-get install php-pear
> +  $ sudo pear config-set auto_discover 1
> +  $ sudo pear install pear.phpunit.de/PHPUnit
> +
> +Then to run the test from the 'testing' directory:
> +$ cd testing/
> +$ phpunit LicenseHelperTest

Indent these as well, please :)

> === added file 'licenses/LicenseHelper.php'

Looks so much better, thanks..

> === added file 'testing/LicenseHelperTest.php'
> --- testing/LicenseHelperTest.php	1970-01-01 00:00:00 +0000
> +++ testing/LicenseHelperTest.php	2012-05-08 14:30:26 +0000
> @@ -0,0 +1,175 @@
> +<?php
> +
> +class LicenseHelperTest extends PHPUnit_Framework_TestCase
> +{
> +
> +    private $temp_filename;
> +
> +    /**
> +     * Include test class, create some help files for testing.
> +     */
> +    protected function setUp()
> +    {
> +        require_once("../licenses/LicenseHelper.php");
> +        $this->temp_filename = tempnam(dirname(__FILE__), "unittest");
> +    }

Why don't we get rid of the setUp function and move the require_once to
the top of the file, and temp_filename to the single test that requires
it?

> +
> +    /**
> +     * Remove helper files used in testing.
> +     */
> +    protected function tearDown()
> +    {
> +        unlink($this->temp_filename);
> +    }

This will now try to unlink temp_filename even if it's only created in a
single test.  Move this to that test, and get rid of th tearDown()
function.

> +    /**
> +     * Running checkFile on a symbolic link to an existing file returns true.
> +     */
> +    public function test_checkFile_link()
> +    {
> +        try {
> +            symlink($this->temp_filename, "test_link");
> +            $this->assertTrue(LicenseHelper::checkFile("test_link"));
> +            unlink("test_link");
> +            // PHP doesn't support finally block, ergo using this hack.
> +        } catch (Exception $e) {
> +            unlink("test_link");
> +            throw $e;
> +        }
> +    }

Where's that test for the checkFile on a link to a non-existent file?
Perhaps PHP symlink(tempnam(), "test_link") doesn't work?

> +    /**
> +     * getFileList returns a list of filenames in that directory.
> +     */
> +    public function test_getFilesList_dir()
> +    {
> +        $temp_dir_name = tempnam(dirname(__FILE__), "unittest");
> +        if (file_exists($temp_dir_name)) {
> +            unlink($temp_dir_name);
> +        }
> +        mkdir($temp_dir_name);
> +
> +        $temp_file_name_1 = tempnam($temp_dir_name, "unittest");
> +        $temp_file_name_2 = tempnam($temp_dir_name, "unittest");
> +
> +        try {
> +            $file_list = LicenseHelper::getFilesList($temp_dir_name);
> +            $this->assertCount(2, $file_list);
> +
> +            $this->assertEquals(basename($temp_file_name_1), $file_list[0]);
> +            $this->assertEquals(basename($temp_file_name_2), $file_list[1]);
> +
> +            unlink($temp_file_name_1);
> +            unlink($temp_file_name_2);
> +            rmdir($temp_dir_name);
> +            // PHP doesn't support finally block, ergo using this hack.
> +        } catch (Exception $e) {
> +            unlink($temp_file_name_1);
> +            unlink($temp_file_name_2);
> +            rmdir($temp_dir_name);
> +            throw $e;
> +        }
> +    }

We've discussed this on IRC: the test is not stable, since
temp_file_name_1 and temp_file_name_2 can end up being named
differently, and file_list can return them in any order.

Best solution for this is to use static names for them (you are already
creating a temp directory to put them in), when you know what ordering
to expect the files in.

As we agreed, if the tempdir is in a system temp dir, we can also get
rid of the finally hack and let stuff stay there when a test fails.

> +
> +    /**
> +     * getTheme returns a generic Linaro-branded template when
> +     * no EULA is present (indicated by eula filename being named
> +     * EULA.txt or not).

How's "ste" representing a "generic" Linaro-branded template?  This is
for files containing "snowball" in a name, where we return
"ste" (ST-Ericsson).

> +     */
> +    public function test_getTheme_noEula_snowball()
> +    {
> +        $eula = "EULA.txt";
> +        $filename = "snowball.build.tar.bz2";
> +        $this->assertEquals("ste", LicenseHelper::getTheme($eula, $filename));
> +    }
> +
> +    /**
> +     * getTheme returns a generic Linaro-branded template when
> +     * no EULA is present (indicated by eula filename being named
> +     * EULA.txt or not).
> +     */

More copy-paste :)

> +    public function test_getTheme_noEula_origen()
> +    {
> +        $eula = "EULA.txt";
> +        $filename = "origen.build.tar.bz2";
> +        $this->assertEquals("samsung",
> +                            LicenseHelper::getTheme($eula, $filename));
> +    }
> +
> +    /**
> +     * getTheme returns a generic Linaro-branded template when
> +     * no EULA is present (indicated by eula filename being named
> +     * EULA.txt or not).
> +     */

There we go, this one is fine :)

> +    public function test_getTheme_noEula_generic()
> +    {
> +        $eula = "EULA.txt";
> +        $filename = "build.tar.bz2";
> +        $this->assertEquals("linaro",
> +                            LicenseHelper::getTheme($eula, $filename));
> +    }
> +
> +    /**
> +     * Running geTheme with eula file present (indicated by eula filename

Typo: getTheme.

> +     * being named EULA.txt or not) returns extension of eula file.
> +     */
> +    public function test_getTheme_eula()
> +    {
> +        $eula = "EULA.txt.test";
> +        $this->assertEquals("test", LicenseHelper::getTheme($eula, ""));
> +    }
> +
> +}
> +
> +?>


Cheers,
Danilo