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

[Stevan Radaković]

Reorganize the code in license.php so the unit test in phpunit can be implemented.
Implement phpunit tests for all the functions from license.php.
Refactor functions where we have tests failing.
Update README so the unit tests are properly documented.

[Georgy Redkozubov (gesha)]

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.

review: Needs Fixing

[Georgy Redkozubov (gesha)]

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]);

review: Needs Fixing

[Stevan Radaković (stevanr)]

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

[Georgy Redkozubov (gesha)]

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

[Stevan Radaković (stevanr)]

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

[Stevan Radaković (stevanr)]

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

[Stevan Radaković (stevanr)]

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

[Данило Шеган (danilo)]

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.

[Данило Шеган (danilo)]

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

review: Needs Fixing

[Stevan Radaković (stevanr)]

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

[Данило Шеган (danilo)]

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

[Stevan Radaković (stevanr)]

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

[Данило Шеган (danilo)]

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.

[Данило Шеган (danilo)]

Excellent, thanks a lot for working on this.

review: Approve

[lp:~stevanr/linaro-license-protection/phpunit-tests]

78. By Stevan Radaković on 2012-05-09

Add file list sorting in getFilesList test.

[Данило Шеган (danilo)]

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

=== modified file 'README'
--- README	2012-05-04 17:44:47 +0000
+++ README	2012-05-09 07:56:17 +0000
@@ -82,3 +82,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

=== added file 'licenses/LicenseHelper.php'
--- licenses/LicenseHelper.php	1970-01-01 00:00:00 +0000
+++ licenses/LicenseHelper.php	2012-05-09 07:56:17 +0000
@@ -0,0 +1,102 @@
+<?php
+
+class LicenseHelper
+{
+
+    /**
+     * 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)
+    {
+        if (is_file($fn) or is_link($fn)) {
+            return true;
+        }
+        return false;
+    }
+
+    /**
+     * Get list of filenames from a directory
+     */
+    public static function getFilesList($dirname)
+    {
+        if (!is_dir($dirname)) {
+            throw new InvalidArgumentException('Method argument '.
+                                               'should be a directory path');
+        }
+
+        $files = array();
+        if ($handle = opendir($dirname)) {
+            while ($handle && false !== ($entry = readdir($handle))) {
+                if ($entry != "." && $entry != ".." &&
+                    !is_dir($dirname."/".$entry) && $entry != "HEADER.html") {
+                    $files[] = $entry;
+                }
+            }
+        }
+        closedir($handle);
+        return $files;
+    }
+
+    /**
+     * Find a matching filename in an array from given filename template.
+     */
+    public static function findFileByPattern($fl, $pattern)
+    {
+        if (!empty($fl)) {
+            foreach ($fl as $f) {
+                if (preg_match($pattern, $f, $matches)) {
+                    return $f;
+                }
+            }
+        }
+        return false;
+    }
+
+    /**
+     * Get license theme name from EULA filename.
+     */
+    public static function getTheme($eula, $down)
+    {
+        if ($eula != 'EULA.txt') { // Special EULA file was found
+            $theme = array_pop(explode(".", $eula));
+        } else { // No special EULA file was found
+            $eula = "EULA.txt";
+            if (preg_match("/.*snowball.*/", $down)) {
+                $theme = "ste";
+            } elseif (preg_match("/.*origen.*/", $down)) {
+                $theme = "samsung";
+            } else {
+                $theme = "linaro";
+            }
+        }
+        return $theme;
+    }
+
+    public static function status_forbidden($dir)
+    {
+        header("Status: 403");
+        header("HTTP/1.1 403 Forbidden");
+        echo "<h1>Forbidden</h1>";
+        echo "You don't have permission to access ".$dir." on this server.";
+        exit;
+    }
+
+    public static function status_ok($dir, $domain)
+    {
+        header("Status: 200");
+        header("Location: ".$dir);
+        setcookie("redirectlicensephp", "yes", 0, "/", ".".$domain);
+        exit;
+    }
+
+    public static function status_not_found()
+    {
+        header("Status: 404");
+        header("HTTP/1.0 404 Not Found");
+        echo "<h1>404 Not Found</h1>";
+        echo "The requested URL was not found on this server.";
+        exit;
+    }
+
+}
\ No newline at end of file

=== modified file 'licenses/license.php'
--- licenses/license.php	2012-05-02 10:56:00 +0000
+++ licenses/license.php	2012-05-09 07:56:17 +0000
@@ -1,97 +1,6 @@
 <?php
-// Get list of files into array to process them later.
-// Used to find special licenses and dirs with only subdirs.
-function check_file($fn)
-{
-	if (is_file($fn) or is_link($fn)) {
-		return true;
-	}
-	return false;
-}
-
-function getFilesList($dirname)
-{
-	$files = array(); 
-	if ($handle = opendir($dirname)) {
-		while ($handle && false !== ($entry = readdir($handle))) {
-			if ($entry != "." && $entry != ".." && !is_dir($dirname."/".$entry) && $entry != "HEADER.html") {
-				$files[] = $entry;
-			}
-		}
-	}
-	closedir($handle);
-	return $files;
-}
-
-// Get array of file name and extension from full filename.
-function splitFilename($filename)
-{
-	$pos = strpos($filename, '.');
-	if ($pos === false) { // dot is not found in the filename
-		return array($filename, ''); // no extension
-	} else {
-		$basename = substr($filename, 0, $pos);
-		$extension = substr($filename, $pos+1);
-		return array($basename, $extension);
-	}
-}
-
-// Find special EULA based on filename template.
-function findSpecialEULA($fl, $pattern)
-{
-	if (!empty($fl)) {
-		foreach ($fl as $f) {
-			if (preg_match($pattern, $f, $matches)) {
-				return $f;
-			}
-		}
-	}
-       	return false;
-}
-
-// Get license theme name from EULA filename.
-function getTheme($eula, $down)
-{
-	if ($eula != 'EULA.txt') { // Special EULA file was found
-		$theme = array_pop(explode(".", $eula));
-	} else { // No special EULA file was found
-		$eula = "EULA.txt";
-		if (preg_match("/.*snowball.*/", $down)) {
-			$theme = "ste";
-		} elseif (preg_match("/.*origen.*/", $down)) {
-			$theme = "samsung";
-		} else {
-			$theme = "linaro";
-		}
-	}
-	return $theme;
-}
-
-function status_forbidden($dir)
-{
-	header("Status: 403");
-	header("HTTP/1.1 403 Forbidden");
-	echo "<h1>Forbidden</h1>";
-	echo "You don't have permission to access ".$dir." on this server.";
-	exit;
-}
-
-function status_ok($dir, $domain)
-{
-	header("Status: 200");
-	header("Location: ".$dir);
-	setcookie("redirectlicensephp", "yes", 0, "/", ".".$domain);
-	exit;
-}
-
-function status_not_found()
-{
-	header("Status: 404");
-	header("HTTP/1.0 404 Not Found");
-	echo "<h1>404 Not Found</h1>";
-	echo "The requested URL was not found on this server.";
-	exit;
-}
+
+require_once("LicenseHelper.php");
 
 $down = $_COOKIE["downloadrequested"];
 $host = $_SERVER["HTTP_HOST"];
@@ -102,10 +11,10 @@
 $eula = '';
 
 if (preg_match("/.*openid.*/", $fn) or preg_match("/.*restricted.*/", $fn) or preg_match("/.*private.*/", $fn)) {
-	status_ok($down, $domain);
+	LicenseHelper::status_ok($down, $domain);
 }
 
-if (file_exists($fn) and check_file($fn)) { // Requested download is file
+if (file_exists($fn) and LicenseHelper::checkFile($fn)) { // Requested download is file
 	$search_dir = dirname($fn);
 	$repl = dirname($down);
 	$name_only = array(basename($down), '');
@@ -114,36 +23,36 @@
 	$repl = $down;
 	$name_only = array();
 } else { // Requested download not found on server
-	status_not_found();
+	LicenseHelper::status_not_found();
 }
 
-$flist = getFilesList($search_dir);
+$flist = LicenseHelper::getFilesList($search_dir);
 
 if (!empty($name_only)) {
 	$pattern = "/^".$name_only[0]."\.EULA\.txt.*/";
-	$eula = findSpecialEULA($flist, $pattern);
+	$eula = LicenseHelper::findFileByPattern($flist, $pattern);
 }
 
-if (check_file($fn)) {
-	if (check_file($doc."/".$repl."/".$eula)) { // Special EULA found
-		$theme = getTheme($eula, $down);
-	} elseif (check_file($doc."/".$repl."/EULA.txt")) { // No special EULA found
-		$theme = getTheme("EULA.txt", $down);
-	} elseif (findSpecialEULA($flist, "/.*EULA.txt.*/")) {
+if (LicenseHelper::checkFile($fn)) {
+	if (LicenseHelper::checkFile($doc."/".$repl."/".$eula)) { // Special EULA found
+		$theme = LicenseHelper::getTheme($eula, $down);
+	} elseif (LicenseHelper::checkFile($doc."/".$repl."/EULA.txt")) { // No special EULA found
+		$theme = LicenseHelper::getTheme("EULA.txt", $down);
+	} elseif (LicenseHelper::findFileByPattern($flist, "/.*EULA.txt.*/")) {
 		// If file is requested but no special EULA for it and no EULA.txt is present,
 		// look for any EULA and if found decide that current file is not protected.
-		status_ok($down, $domain);
+		LicenseHelper::status_ok($down, $domain);
 	} else {
-		status_forbidden($down);
+		LicenseHelper::status_forbidden($down);
 	}
 } elseif (is_dir($fn)) {
-	if (empty($flist) or findSpecialEULA($flist, "/.*EULA.txt.*/")) { // Directory contains only subdirs or any EULA
-		status_ok($down, $domain);
+	if (empty($flist) or LicenseHelper::findFileByPattern($flist, "/.*EULA.txt.*/")) { // Directory contains only subdirs or any EULA
+		LicenseHelper::status_ok($down, $domain);
 	} else { // No special EULA, no EULA.txt, no OPEN-EULA.txt found
-		status_forbidden($down);
+		LicenseHelper::status_forbidden($down);
 	}
 } else {
-	status_forbidden($down);
+	LicenseHelper::status_forbidden($down);
 }
 
 $template_content = file_get_contents($doc."/licenses/".$theme.".html");

=== added file 'testing/LicenseHelperTest.php'
--- testing/LicenseHelperTest.php	1970-01-01 00:00:00 +0000
+++ testing/LicenseHelperTest.php	2012-05-09 07:56:17 +0000
@@ -0,0 +1,167 @@
+<?php
+
+require_once("../licenses/LicenseHelper.php");
+
+class LicenseHelperTest extends PHPUnit_Framework_TestCase
+{
+
+    private $temp_filename;
+
+    /**
+     * Running checkFile on a directory path returns false.
+     */
+    public function test_checkFile_nonFile()
+    {
+        $this->assertFalse(LicenseHelper::checkFile(dirname(__FILE__)));
+    }
+
+    /**
+     * Running checkFile on a symbolic link to an existing file returns true.
+     */
+    public function test_checkFile_link()
+    {
+        $this->temp_filename = tempnam(sys_get_temp_dir(), "unittest");
+        symlink($this->temp_filename, "test_link");
+
+        $this->assertTrue(LicenseHelper::checkFile("test_link"));
+
+        unlink("test_link");
+        unlink($this->temp_filename);
+    }
+
+    /**
+     * Running checkFile on a broken symbolic link returns true.
+     * This is because PHP function is_link returns true on broken soft links.
+     */
+    public function test_checkFile_brokenLink()
+    {
+        $this->temp_filename = tempnam(sys_get_temp_dir(), "unittest");
+        symlink($this->temp_filename, "test_link");
+        unlink($this->temp_filename);
+
+        $this->assertTrue(LicenseHelper::checkFile("test_link"));
+
+        unlink("test_link");
+    }
+
+    /**
+     * Running checkFile on a regular file returns true.
+     */
+    public function test_checkFile_file()
+    {
+        $this->assertTrue(LicenseHelper::checkFile(__FILE__));
+    }
+
+    /**
+     * getFileList throws an InvalidArgumentException when passed
+     * an argument pointing to a file.
+     * @expectedException InvalidArgumentException
+     */
+    public function test_getFilesList_file()
+    {
+        $file_list = LicenseHelper::getFilesList(__FILE__);
+    }
+
+    /**
+     * getFileList returns a list of filenames in that directory.
+     */
+    public function test_getFilesList_dir()
+    {
+        $temp_dir_name = tempnam(sys_get_temp_dir(), "unittest");
+        if (file_exists($temp_dir_name)) {
+            unlink($temp_dir_name);
+        }
+        mkdir($temp_dir_name);
+
+        touch($temp_dir_name . "/unittest1");
+        touch($temp_dir_name . "/unittest2");
+
+        $file_list = LicenseHelper::getFilesList($temp_dir_name);
+        $this->assertCount(2, $file_list);
+
+        // Sort the file list, this function returns the files as they are
+        // written on the filesystem.
+        sort($file_list);
+        $this->assertEquals("unittest1", $file_list[0]);
+        $this->assertEquals("unittest2", $file_list[1]);
+
+        unlink($temp_dir_name . "/unittest1");
+        unlink($temp_dir_name . "/unittest2");
+        rmdir($temp_dir_name);
+    }
+
+    /**
+     * Running findFileByPattern on an array without matches returns false.
+     */
+    public function test_findFileByPattern_noMatch()
+    {
+        $file_list = array("test.txt", "new_file.pdf");
+        $pattern = "/^abc/";
+        $this->assertFalse(
+            LicenseHelper::findFileByPattern($file_list, $pattern));
+    }
+
+    /**
+     * Running findFileByPattern on an array with matches returns first
+     * matching element.
+     */
+    public function test_findFileByPattern_match()
+    {
+        $file_list = array("test.txt", "new_file.pdf");
+        $pattern = "/test/";
+        $this->assertEquals("test.txt",
+                            LicenseHelper::findFileByPattern($file_list,
+                                                             $pattern));
+    }
+
+    /**
+     * getTheme returns a ST-E Linaro-branded template when
+     * no EULA is present (indicated by eula filename being named
+     * EULA.txt or not).
+     */
+    public function test_getTheme_noEula_snowball()
+    {
+        $eula = "EULA.txt";
+        $filename = "snowball.build.tar.bz2";
+        $this->assertEquals("ste", LicenseHelper::getTheme($eula, $filename));
+    }
+
+    /**
+     * getTheme returns a Samsung Linaro-branded template when
+     * no EULA is present (indicated by eula filename being named
+     * EULA.txt or not).
+     */
+    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).
+     */
+    public function test_getTheme_noEula_generic()
+    {
+        $eula = "EULA.txt";
+        $filename = "build.tar.bz2";
+        $this->assertEquals("linaro",
+                            LicenseHelper::getTheme($eula, $filename));
+    }
+
+    /**
+     * Running getTheme with eula file present (indicated by eula filename
+     * 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, ""));
+    }
+
+}
+
+?>
\ No newline at end of file