This change updates PhpunitTestEngine.php and ArcanistPhpunitTestResultParser.php to use phpunit's junit output rather than json.
--log-json was deprecated in PHPUnit 5.7, and removed in PHPUnit 6.0.0 (2017-02-03).
Fixes T15667
Details
Details
- Reviewers
avivey speck - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15667: Update PhpunitTestEngine to not call phpunit with --log-json option
- Commits
- rARC7c5e607e9752: Update PhpunitTestEngine.php and ArcanistPhpunitTestResultParser.php to use…
Download an example PHP repo, with arcconfig set to 'PhpunitTestEngine'. Example:
git clone https://github.com/campbsb/example-phorge-php-project.git
Install phpunit using composer, and test it
cd example-phorge-php-project php ~/composer.phar update phpunit
In a PHP phorge repo, set the following in .arcconfig:
"unit.engine": "PhpunitTestEngine", "phpunit_config": "phpunit.xml",
Make a non-breaking change to src/Service.php (eg add a space), and run 'arc unit'
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/unit/parser/ArcanistPhpunitTestResultParser.php | ||
|---|---|---|
| 27–28 | There are some arguments we probably want to pass into this function call, namely the third argument, https://www.php.net/manual/en/function.simplexml-load-string.php At minimum we should consider LIBXML_NONET to avoid the xml parsing from making any network activity, for sake of security. Several of the other options look applicable/interesting though probably not as critical. | |
| 40 | Does $error include the file and line information? Same q w/ above for $failure. | |
| 40–41 | Is the comment here still applicable? Also should this do the same preg_replace operation as above to compute $name? | |
Comment Actions
Just as a reference point, a few years ago, I created a version of this as well - It supports readCoverage coverage reports, etc.
https://github.com/lucit-cc/PhpunitJunitTestEngine/tree/master
I don't have a diff - which is unfortunate, but maybe looking at the code I wrote might make sure nothing is missed?
I can try to find some time to review later as well
Comment Actions
- Add LIBXML_NONET option
- Remove $last_test_finished and the !last_test_finished block, as that is always true
Comment Actions
Testing, using https://github.com/campbsb/example-phorge-php-project.git and breaking it so as to cause a test failure, we get:
$ arc unit FAIL Example\PhorgeProjectTests\ServiceTest Example\PhorgeProjectTests\ServiceTest::testReturnsTrue Check returnsTrue () returns true Failed asserting that false is identical to true. /home/sten/example-phorge-php-project/tests/ServiceTest.php:23
which demonstrates that we see the file and line number of the failing test.
Replacing this diff's PhpunitTestEngine with @Ekubischta's https://github.com/lucit-cc/PhpunitJunitTestEngine, we get:
$ arc unit FAIL [ ServiceTest.php ] testReturnsTrue Example\PhorgeProjectTests\ServiceTest::testReturnsTrue Check returnsTrue () returns true Failed asserting that false is identical to true. /home/sten/example-phorge-php-project/tests/ServiceTest.php:23
Which presents the same information, marginally nicer.
| src/unit/parser/ArcanistPhpunitTestResultParser.php | ||
|---|---|---|
| 40 | We do get file and line information | |
Comment Actions
I'm just glossing over this - is ArcanistPhpunitTestResultParser now learning to parse generic "junit style xml" format?
Comment Actions
@speck , @valerio.bozzolan
Sorry, yes, it's updating the test engine to parse phpunit's junit output, rather than the json output which no longer exists.
Comment Actions
So maybe rename the parser to JUnitTestResult, or something like that (If that's the right name for the format and if phpunit is using the same format)?
Comment Actions
As per @avivey, update ArcanistPhpunitTestResultParser.php to call ArcanistXUnitTestResultParser and remove it's own XUnit parsing code.
Comment Actions
Possible further enhancements, which I am loathe to do when trying to fix an existing bug, so perhaps as a future diff:
- Update ArcanistXUnitTestResultParser->parseTestResults() to take a second argument, $path, which, if provided and $test_results is empty, will mimic the ArcanistPhpunitTestResultParser->parseTestResults() behaviour in returning a RESULT_BROKEN ArcanistUnitTestResult. I find this preferable to throwing an exception.
- Move the ArcanistPhpunitTestResultParser->readCoverage() into a new ArcanistCloverCoverageParser.php
Doing both of these would mean that ArcanistPhpunitTestResultParser.php could be reduced to just a few lines of code, and arguably incorporated entirely into PhpunitTestEngine.php