Merge lp://staging/~stevanr/linaro-license-protection/phpunit-tests into lp://staging/~linaro-automation/linaro-license-protection/trunk

The behaviour of click through license protection has slightly changed. As results following actions should be done:
1) Update filefetcher.py module in tests, now it is not in the branch.
2) Write new tests for "empty" directories handling.
3) Update old tests to use new filefetcher.py module.

1) Please revert your changes in 'testing/test_click_through_license.py' back.
2) Add instructions how to install PHP_CodeCoverage, I had to follow following steps to get running your test:
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
3) You have incorrect expected value for file.with.more.dots, should be:
$this->assertEquals("file", $filename_array[0]);
$this->assertEquals("with.more.dots", $filename_array[1]);

> 1) Please revert your changes in 'testing/test_click_through_license.py' back.
> 2) Add instructions how to install PHP_CodeCoverage, I had to follow following
> steps to get running your test:
> 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
> 3) You have incorrect expected value for file.with.more.dots, should be:
> $this->assertEquals("file", $filename_array[0]);
> $this->assertEquals("with.more.dots", $filename_array[1]);

2) I was able to do the tests on fresh 12.04 only by installing phpunit package. Maybe we should discuss this.
3) The test actually shows that the code should be refactored since the extension of 'file.with.more.dots' would be 'dots' and not 'with.more.dots'.

> > 1) Please revert your changes in 'testing/test_click_through_license.py'
> back.
> > 2) Add instructions how to install PHP_CodeCoverage, I had to follow
> following
> > steps to get running your test:
> > 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
> > 3) You have incorrect expected value for file.with.more.dots, should be:
> > $this->assertEquals("file", $filename_array[0]);
> > $this->assertEquals("with.more.dots", $filename_array[1]);
>
> 2) I was able to do the tests on fresh 12.04 only by installing phpunit
> package. Maybe we should discuss this.

Anyway lets add information about if the execution of tests fails with PHP_CodeCoverage error

> 3) The test actually shows that the code should be refactored since the
> extension of 'file.with.more.dots' would be 'dots' and not 'with.more.dots'.

How about *.tar.bz2?

> > > 1) Please revert your changes in 'testing/test_click_through_license.py'
> > back.
> > > 2) Add instructions how to install PHP_CodeCoverage, I had to follow
> > following
> > > steps to get running your test:
> > > 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
> > > 3) You have incorrect expected value for file.with.more.dots, should be:
> > > $this->assertEquals("file", $filename_array[0]);
> > > $this->assertEquals("with.more.dots", $filename_array[1]);
> >
> > 2) I was able to do the tests on fresh 12.04 only by installing phpunit
> > package. Maybe we should discuss this.
>
> Anyway lets add information about if the execution of tests fails with
> PHP_CodeCoverage error

Ok

>
> > 3) The test actually shows that the code should be refactored since the
> > extension of 'file.with.more.dots' would be 'dots' and not 'with.more.dots'.
>
> How about *.tar.bz2?

Good point.. now I'm out of ideas atm.

> > > 1) Please revert your changes in 'testing/test_click_through_license.py'
> > back.
> > > 2) Add instructions how to install PHP_CodeCoverage, I had to follow
> > following
> > > steps to get running your test:
> > > 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
> > > 3) You have incorrect expected value for file.with.more.dots, should be:
> > > $this->assertEquals("file", $filename_array[0]);
> > > $this->assertEquals("with.more.dots", $filename_array[1]);
> >
> > 2) I was able to do the tests on fresh 12.04 only by installing phpunit
> > package. Maybe we should discuss this.
>
> Anyway lets add information about if the execution of tests fails with
> PHP_CodeCoverage error
>
> > 3) The test actually shows that the code should be refactored since the
> > extension of 'file.with.more.dots' would be 'dots' and not 'with.more.dots'.
>
> How about *.tar.bz2?

PHP function pathinfo returns only the last part of the filename with multiple dots, i.e. in example you provided it will return only bz2. Since it's a standard PHP function my opinion is that we should copy it's behavior.

> > > > 1) Please revert your changes in 'testing/test_click_through_license.py'
> > > back.
> > > > 2) Add instructions how to install PHP_CodeCoverage, I had to follow
> > > following
> > > > steps to get running your test:
> > > > 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
> > > > 3) You have incorrect expected value for file.with.more.dots, should be:
> > > > $this->assertEquals("file", $filename_array[0]);
> > > > $this->assertEquals("with.more.dots", $filename_array[1]);
> > >
> > > 2) I was able to do the tests on fresh 12.04 only by installing phpunit
> > > package. Maybe we should discuss this.
> >
> > Anyway lets add information about if the execution of tests fails with
> > PHP_CodeCoverage error
> >
> > > 3) The test actually shows that the code should be refactored since the
> > > extension of 'file.with.more.dots' would be 'dots' and not
> 'with.more.dots'.
> >
> > How about *.tar.bz2?
>
> PHP function pathinfo returns only the last part of the filename with multiple
> dots, i.e. in example you provided it will return only bz2. Since it's a
> standard PHP function my opinion is that we should copy it's behavior.

I've just realized that this function is not used at all in license.php, and basically there are some standard library functions in PHP that already do this task so I think we should be OK if we remove this method as part of the refactoring. What do you think?

Thanks for working on this, Stevan.

First of, please try to follow the existing coding style for the code you are modifying. Existing code is using tabs (uhm, we should change that to 4-spaces like we do in the rest of Infrastructure projects, but that's a separate branch: do not, under any circumstances, do it in this branch).

For the python test failures you are seeing, please file a bug if there isn't one already (on the https://launchpad.net/linaro-license-protection project: there isn't one, so please do that).

Unfortunately, phpunit package is broken in Debian/Ubuntu (bug #701544), so we will have to include the workaround installation in the documentation until package itself is fixed (I expect it might only be fixed in 12.10, unless someone invests a lot of effort in fixing the 12.04 package as well, though that might be worth it considering it's an LTS).

For the function that is not used, please remove it altogether.

Next, I'll comment on the code changes directly.

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)
> + {
> + ...

> 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.
>
> > +{
> > +
...

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()
> +...

> 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 t...

Sorry: I missed this (I thought you just mistakenly quoted the entire
email). Can I ask you to cut out everything but the relevant bit when
replying, please?

У уто, 08. 05 2012. у 14:36 +0000, Stevan Radaković пише:
> >
> > "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?
> >
>
> Very good point. Unfortunately, I'm not sure what was the authors expected behavior for this method (suggestion below), but here's how is_file and is_link work for links and files in PHP.
>
> is_file(file) - TRUE
> is_file(hard_link) - TRUE
> is_file(soft_link) - TRUE
> is_file(broken_soft_link) - FALSE
>
> is_link(file) - FALSE
> is_link(hard_link) - FALSE
> is_link(soft_link) - TRUE
> is_link(broken_soft_link) - TRUE
>
> Some of those are really odd, and in my opinion we should only use the standard is_file function, because it basically serves to identify files and links from directories.

For exactly the reason you are not sure what the "expected behavior for
this method" is, we should document it. Let's add a test which confirms
the current behavior, and it will serve as documentation in the future
as well.

Excellent, thanks a lot for working on this.

Ok, there was another test instability (well, the same one). That's now fixed.