Page MenuHomePhabricator

Arc unit command fails with PHPUnit 6
Open, Needs TriagePublic

Description

After updating unit tests to be compatible with PHPUnit 6, arc unit command fails what leads to inability to create a differential revision.

Reproduction steps

  1. create new directory, init git and set up necessary files:
composer.json
{
    "require": {
        "phpunit/phpunit": "^6.0"
    }
}
.arcconfig
{
    "unit.engine": "PhpunitTestEngine",
    "unit.phpunit.binary": "vendor/bin/phpunit"
}
dataTest.php
<?php
use PHPUnit\Framework\TestCase;

class DataTest extends TestCase
{
    public function testAdd()
    {
        $this->assertEquals(4, 2 + 2);
    }
}
  1. run composer install
  2. run arc unit dataTest.php --trace:
13:41 $ arc unit dataTest.php --trace
...
>>> [0] <exec> $ which 'vendor/bin/phpunit'
<<< [0] <exec> 2,203 us
>>> [1] <exec> $ vendor/bin/phpunit  -d display_errors=stderr --log-json '/tmp/801ns8shcgkc00k0/17490-AUKPdN' --coverage-clover '/tmp/47h5jepdu2as084s/17490-QK7yko' '/home/ali/arctest/dataTest.php'
<<< [1] <exec> 37,234 us
   BROKEN  /home/ali/arctest/dataTest.php

Test ^5 Unit test version

run the following commands:

rm -rf vendors
rm composer.lock
sed -i 's/6.0/5.0/g' composer.json
composer install
arc unit dataTest.php
13:45 $ arc unit dataTest.php 
   PASS    2ms★  DataTest::testAdd

The issue

PHPUnit changelog for version 6.0 says:

* Removed the `--log-json` commandline option (deprecated in PHPUnit 5.7)

This command line option is used in standard PhpunitTestEngine:
https://github.com/phacility/arcanist/blob/9e82ef979e8148c43b9b8439025d505b1219e213/src/unit/engine/PhpunitTestEngine.php#L67

Arc version

arcanist 129d51fa0936c9bae48fadf3a3f39e26d69d24da (18 May 2017)
libphutil a900d7b63e954e221efe140f0f33d3d701524aae (23 Apr 2017)

Event Timeline

albrk created this task.Jun 1 2017, 10:54 AM
albrk renamed this task from Arc unit fails with PHPUnit 6 to Arc unit command fails with PHPUnit 6.Jun 1 2017, 12:33 PM
albrk updated the task description. (Show Details)
tkjn awarded a token.Jun 1 2017, 12:54 PM
tkjn added a subscriber: tkjn.

Looks like it was deprecated a while ago, without explanation. Luckily, there's --log-junit, which we have some code to parse.

Also, see T10038.

aurelijus added a subscriber: aurelijus.

So, to recap:

  • PhpunitTestEngine::run must be updated to use --log-junit which will output XML instead of JSON
  • ArcanistPhpunitTestResultParser::parseTestResults must be updated to parse this XML file.

This seems simple enough, I will try to make it work.

@epriestley I tried submitting a patch, but I get a Permission denied (publickey) which results in Unable to push changes to the staging area..
I added my public key and created a config rule to use it for requests to secure.phabricator.com, I'm not sure what else to do.

cmmata added a subscriber: cmmata.Feb 1 2018, 9:07 AM
cmmata added a comment.Feb 1 2018, 9:47 AM

@mcorteel I have the same problem, arc unit only works with phpunit 5.7. If I use 6.5, it fails. Do you have that patch anywhere with public access so we can use it while it's not published?

@cmmata here is my diff. It can probably be improved, but it works for me:

1diff --git a/src/unit/engine/PhpunitTestEngine.php b/src/unit/engine/PhpunitTestEngine.php
2index 8206b787..ef01fda4 100644
3--- a/src/unit/engine/PhpunitTestEngine.php
4+++ b/src/unit/engine/PhpunitTestEngine.php
5@@ -52,7 +52,7 @@ final class PhpunitTestEngine extends ArcanistUnitTestEngine {
6 if (!Filesystem::pathExists($test_path)) {
7 continue;
8 }
9- $json_tmp = new TempFile();
10+ $xml_tmp = new TempFile();
11 $clover_tmp = null;
12 $clover = null;
13 if ($this->getEnableCoverage() !== false) {
14@@ -64,10 +64,10 @@ final class PhpunitTestEngine extends ArcanistUnitTestEngine {
15
16 $stderr = '-d display_errors=stderr';
17
18- $futures[$test_path] = new ExecFuture('%C %C %C --log-json %s %C %s',
19- $this->phpunitBinary, $config, $stderr, $json_tmp, $clover, $test_path);
20+ $futures[$test_path] = new ExecFuture('%C %C %C --log-junit %s %C %s',
21+ $this->phpunitBinary, $config, $stderr, $xml_tmp, $clover, $test_path);
22 $tmpfiles[$test_path] = array(
23- 'json' => $json_tmp,
24+ 'xml' => $xml_tmp,
25 'clover' => $clover_tmp,
26 );
27 }
28@@ -81,7 +81,7 @@ final class PhpunitTestEngine extends ArcanistUnitTestEngine {
29
30 $results[] = $this->parseTestResults(
31 $test,
32- $tmpfiles[$test]['json'],
33+ $tmpfiles[$test]['xml'],
34 $tmpfiles[$test]['clover'],
35 $stderr);
36 }
37@@ -90,17 +90,17 @@ final class PhpunitTestEngine extends ArcanistUnitTestEngine {
38 }
39
40 /**
41- * Parse test results from phpunit json report.
42+ * Parse test results from phpunit XML report.
43 *
44 * @param string $path Path to test
45- * @param string $json_tmp Path to phpunit json report
46+ * @param string $xml_tmp Path to phpunit XML report
47 * @param string $clover_tmp Path to phpunit clover report
48 * @param string $stderr Data written to stderr
49 *
50 * @return array
51 */
52- private function parseTestResults($path, $json_tmp, $clover_tmp, $stderr) {
53- $test_results = Filesystem::readFile($json_tmp);
54+ private function parseTestResults($path, $xml_tmp, $clover_tmp, $stderr) {
55+ $test_results = Filesystem::readFile($xml_tmp);
56 return id(new ArcanistPhpunitTestResultParser())
57 ->setEnableCoverage($this->getEnableCoverage())
58 ->setProjectRoot($this->projectRoot)
59diff --git a/src/unit/parser/ArcanistPhpunitTestResultParser.php b/src/unit/parser/ArcanistPhpunitTestResultParser.php
60index 5ccff970..3d7fcd77 100644
61--- a/src/unit/parser/ArcanistPhpunitTestResultParser.php
62+++ b/src/unit/parser/ArcanistPhpunitTestResultParser.php
63@@ -9,10 +9,10 @@
64 final class ArcanistPhpunitTestResultParser extends ArcanistTestResultParser {
65
66 /**
67- * Parse test results from phpunit json report
68+ * Parse test results from phpunit XML report
69 *
70 * @param string $path Path to test
71- * @param string $test_results String containing phpunit json report
72+ * @param string $test_results String containing phpunit XML report
73 *
74 * @return array
75 */
76@@ -25,7 +25,7 @@ final class ArcanistPhpunitTestResultParser extends ArcanistTestResultParser {
77 return array($result);
78 }
79
80- $report = $this->getJsonReport($test_results);
81+ $report = simplexml_load_string($test_results);
82
83 // coverage is for all testcases in the executed $path
84 $coverage = array();
85@@ -36,56 +36,36 @@ final class ArcanistPhpunitTestResultParser extends ArcanistTestResultParser {
86 $last_test_finished = true;
87
88 $results = array();
89- foreach ($report as $event) {
90- switch (idx($event, 'event')) {
91- case 'test':
92- break;
93- case 'testStart':
94- $last_test_finished = false;
95- // fall through
96- default:
97- continue 2; // switch + loop
98- }
99-
100+ foreach ($report->testsuite as $test_suite) {
101 $status = ArcanistUnitTestResult::RESULT_PASS;
102 $user_data = '';
103
104- if ('fail' == idx($event, 'status')) {
105+ if ((int)$test_suite['failures'] > 0) {
106 $status = ArcanistUnitTestResult::RESULT_FAIL;
107- $user_data .= idx($event, 'message')."\n";
108- foreach (idx($event, 'trace') as $trace) {
109- $user_data .= sprintf(
110- "\n%s:%s",
111- idx($trace, 'file'),
112- idx($trace, 'line'));
113+ foreach ($test_suite->testcase as $test_case) {
114+ foreach ($test_case->failure as $failure) {
115+ $user_data .= sprintf(
116+ "\n%s",
117+ (string)$failure);
118+ }
119 }
120- } else if ('error' == idx($event, 'status')) {
121- if (strpos(idx($event, 'message'), 'Skipped Test') !== false) {
122- $status = ArcanistUnitTestResult::RESULT_SKIP;
123- $user_data .= idx($event, 'message');
124- } else if (strpos(
125- idx($event, 'message'),
126- 'Incomplete Test') !== false) {
127- $status = ArcanistUnitTestResult::RESULT_SKIP;
128- $user_data .= idx($event, 'message');
129- } else {
130- $status = ArcanistUnitTestResult::RESULT_BROKEN;
131- $user_data .= idx($event, 'message');
132- foreach (idx($event, 'trace') as $trace) {
133- $user_data .= sprintf(
134- "\n%s:%s",
135- idx($trace, 'file'),
136- idx($trace, 'line'));
137+ } else if ($test_suite['errors'] > 0) {
138+ $status = ArcanistUnitTestResult::RESULT_BROKEN;
139+ foreach ($test_suite->testcase as $test_case) {
140+ foreach ($test_case->error as $error) {
141+ $user_data .= sprintf(
142+ "\n%s",
143+ (string)$error);
144 }
145 }
146 }
147
148- $name = preg_replace('/ \(.*\)/s', '', idx($event, 'test'));
149+ $name = preg_replace('/ \(.*\)/s', '', $test_suite['name']);
150
151 $result = new ArcanistUnitTestResult();
152 $result->setName($name);
153 $result->setResult($status);
154- $result->setDuration(idx($event, 'time'));
155+ $result->setDuration((float)$test_suite['time']);
156 $result->setCoverage($coverage);
157 $result->setUserData($user_data);
158
159@@ -95,7 +75,7 @@ final class ArcanistPhpunitTestResultParser extends ArcanistTestResultParser {
160
161 if (!$last_test_finished) {
162 $results[] = id(new ArcanistUnitTestResult())
163- ->setName(idx($event, 'test')) // use last event
164+ ->setName($test_suite['name']) // use last event
165 ->setUserData($this->stderr)
166 ->setResult(ArcanistUnitTestResult::RESULT_BROKEN);
167 }
168@@ -161,28 +141,4 @@ final class ArcanistPhpunitTestResultParser extends ArcanistTestResultParser {
169 return $reports;
170 }
171
172- /**
173- * We need this non-sense to make json generated by phpunit
174- * valid.
175- *
176- * @param string $json String containing JSON report
177- * @return array JSON decoded array
178- */
179- private function getJsonReport($json) {
180-
181- if (empty($json)) {
182- throw new Exception(
183- pht(
184- 'JSON report file is empty, it probably means that phpunit '.
185- 'failed to run tests. Try running %s with %s option and then run '.
186- 'generated phpunit command yourself, you might get the answer.',
187- 'arc unit',
188- '--trace'));
189- }
190-
191- $json = preg_replace('/}{\s*"/', '},{"', $json);
192- $json = '['.$json.']';
193- return phutil_json_decode($json);
194- }
195-
196 }

cmmata added a comment.Feb 2 2018, 5:01 PM

Thanks @mcorteel, I'll take a look.

I confirm the Maxime Corteel patch to be working with PHPUnit 7.0.1 too.

DragonBe added a subscriber: DragonBe.

Confirmed on PHPUnit 7.2.2 👍

mbi added a subscriber: mbi.Oct 23 2018, 9:54 AM

@epriestley
Support for PHPUnit 5.x ended on February 2, 2018.
We need this patch for PHPUnit 6/7 ... any idea when it can be landed on master?

@epriestley Does this mean that phabricator isn't open to contributions?

I have a proposed patch for which I have tried to submit a review (see T12785#233328).
It probably needs changes to be mergeable (like discussed in P2084), but I'm very open to the idea of writing all the needed code if that's possible.

epriestley added a comment.EditedOct 23 2018, 1:31 PM

It's "open" in the sense of Apache-licensed and (mostly) developed in public (e.g., the information on this install is public rather than hidden behind a corporate veil).

It's not "open" in the sense of "community-driven" or "easy to contribute to". The barrier to actually contributing code is extremely high. From git log, only 15 changes this year came from external contributors (and even those contributors are mostly special in some way).

Per T10038, linked above, I currently plan to remove this code rather than change it.

See also T13196, for example.

Well, this sucks... But thank you for taking the time to explain the situation 😃

For those that don't want to read the whole conversation : you shouldn't rely on arc unit as a PHPUnit wrapper since:

  • PHPUnit 5 isn't maintained and that's all arc unit supports
  • Support for PHPUnit 6/7 won't happen until T10038 is resolved, so don't hold your breath!
Pawka added a subscriber: Pawka.Mar 5 2019, 9:01 AM