Arc unit command fails with PHPUnit 6
Open, Needs TriagePublic
Actions

Assigned To

None

Authored By

	albrk
	Jun 1 2017, 10:54 AM

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

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

run composer install
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)

Related Objects

Mentioned In: P2084 T12785 patch
Mentioned Here: T13196: Allow search cluster path value point to alias for Elasticsearch fulltext engine
P2084 T12785 patch
T10038: Plan the mid-term pathway for unit-test/linter bindings

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 awarded a token.Jun 3 2017, 5:52 AM

aurelijus added a subscriber: aurelijus.

mcorteel added a subscriber: mcorteel.Nov 21 2017, 11:20 AM

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.

jaydiablo added a subscriber: jaydiablo.Jan 31 2018, 10:19 PM

cmmata added a subscriber: cmmata.Feb 1 2018, 9:07 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:

P2084 T12785 patch

1	diff --git a/src/unit/engine/PhpunitTestEngine.php b/src/unit/engine/PhpunitTestEngine.php
2	index 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)
59	diff --git a/src/unit/parser/ArcanistPhpunitTestResultParser.php b/src/unit/parser/ArcanistPhpunitTestResultParser.php
60	index 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	}

Thanks @mcorteel, I'll take a look.

• shekharsharma705 added a subscriber: • shekharsharma705.Feb 12 2018, 6:34 AM

mcorteel mentioned this in P2084 T12785 patch.Feb 21 2018, 4:18 PM

epriestley moved this task from Backlog to Unit/Lint Bindings on the Arcanist board.Mar 5 2018, 2:16 PM

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

DragonBe awarded a token.Jun 7 2018, 1:09 PM

DragonBe added a subscriber: DragonBe.

Confirmed on PHPUnit 7.2.2 👍

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

See Planning.

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

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.

Arc unit command fails with PHPUnit 6Open, Needs TriagePublicActions