Code review comment for lp:~stevanr/linaro-license-protection/phpunit-tests

> 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