У пет, 11. 05 2012. у 15:51 +0000, Stevan Radaković пише:
> Stevan Radaković has proposed merging lp:~stevanr/linaro-license-protection/autorun-php-unittests into lp:linaro-license-protection.
> 
> Requested reviews:
>   Данило Шеган (danilo): code
> 
> For more details, see:
> https://code.launchpad.net/~stevanr/linaro-license-protection/autorun-php-unittests/+merge/105498
> 
> Add new test module with a single test which runs the PHP unit tests and check for failures or errors.
> It uses phpunit 'write results to XML' feature, then parses the XML file for results. 
> разлике међу датотекама прилог (review-diff.txt)
> === modified file 'README'
> --- README	2012-05-11 12:20:01 +0000
> +++ README	2012-05-11 15:50:28 +0000
> @@ -104,5 +104,4 @@
>    $ sudo pear install pear.phpunit.de/PHPUnit
>  
>  Then to run the test from the 'testing' directory:
> -  $ cd testing/
> -  $ phpunit LicenseHelperTest
> +  $ phpunit testing/LicenseHelperTest

Since we are going to be automatically running these tests using 'testr
run', perhaps it would be good to change the narrative to say something
like: To run only the PHP tests using phpunit directly, do the
following.   Though, we might as well remove the entire paragraph, but
I'd rather not until we improve the Python integration to output the
actual failures properly.

> === added file 'testing/test_php_unit.py'
> --- testing/test_php_unit.py	1970-01-01 00:00:00 +0000
> +++ testing/test_php_unit.py	2012-05-11 15:50:28 +0000
> @@ -0,0 +1,33 @@
> +#!/usr/bin/env python

I don't think you need this line.

> +
> +import os
> +import subprocess
> +import xml.etree.ElementTree as etree

It's not customary to rename imported classes/modules, unless they
conflict with a class in the file.  FWIW, someone used to the
ElementTree might confuse the etree for the module instead of the class
and try to do etree.ElementTree inside the code: this adds nothing but
confusion, imho.

Not a big deal, but just thought I'd note my personal opinion.

> +from testtools import TestCase
> +from testtools.matchers import Equals
> +
> +class PhpUnitTest(TestCase):
> +    '''Tests for executing the PHP Unit tests'''
> +
> +    def setUp(self):
> +        super(PhpUnitTest, self).setUp()
> +        self.xml_path = "testing/php_unit_test_result.xml"

As you already know, I hate it when we are writing stuff to the code
directory.  FWIW, one should be able to run tests on a read-only
location, though this project already fails that assumption. For
example, this is very useful for rolling out to production machines,
where IS can trust us not to introduce a bunch of temporary/transient
files in the production location.

> +        if subprocess.Popen(['phpunit', '--log-junit',
> +                             self.xml_path, 'testing/LicenseHelperTest'],
> +                stdout=open('/dev/null', 'w'),
> +                stderr=subprocess.STDOUT).wait():
> +            raise CommandNotFoundException("phpunit command not found. Please "
> +                "install phpunit package and rerun tests.")
> +        self.xml_data = etree.parse(self.xml_path)

This is good, though I have a feeling it would have been cheaper to
parse JSON, which phpunit also seems to offer :)

Not sure if it offers enough data for you to print out though.

> +
> +    def tearDown(self):
> +        super(PhpUnitTest, self).tearDown()
> +        if os.path.exists(self.xml_path):
> +            os.unlink(self.xml_path)
> +
> +    def test_run_php_unit_tests(self):
> +        self.assertThat(self.xml_data.getroot()[0].attrib['failures'],
> +                        Equals("0"))
> +        self.assertThat(self.xml_data.getroot()[0].attrib['errors'],
> +                        Equals("0"))

This looks good.  testtools matchers actually should provide ability to
test for both at the same time (eg. this might fail and indicate that
there are 'failures' without showing that there are 'errors' as well
even if there are).  I don't know exactly how that works, but the
testtools documentation should be able to help.

Again, not a big deal, so overall, I am approving this MP, and you can
choose what (if anything) to change at your own discretion.

  merge approve