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?

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