Merge lp://staging/~stevanr/linaro-license-protection/autorun-php-unittests into lp://staging/~linaro-automation/linaro-license-protection/trunk

Proposed by Stevan Radaković
Status: Merged
Approved by: Данило Шеган
Approved revision: 73
Merged at revision: 73
Proposed branch: lp://staging/~stevanr/linaro-license-protection/autorun-php-unittests
Merge into: lp://staging/~linaro-automation/linaro-license-protection/trunk
Diff against target: 77 lines (+40/-4)
4 files modified
README (+2/-3)
testing/LicenseHelperTest.php (+1/-1)
testing/__init__.py (+1/-0)
testing/test_php_unit.py (+36/-0)
To merge this branch: bzr merge lp://staging/~stevanr/linaro-license-protection/autorun-php-unittests
Reviewer Review Type Date Requested Status
Данило Шеган (community) Approve
Review via email: mp+105498@code.staging.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Данило Шеган (danilo) wrote :
Download full text (4.1 KiB)

У пет, 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 php...

Read more...

review: Approve
74. By Stevan Radaković

Small updates from danilos code review.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches