> 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?
> 

I've already answered to this one on your previous review report. The main point is that PHP function is_link still returns TRUE on broken links.

> > +    /**
> > +     * 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