Page MenuHomePhabricator
Paste P2084

T12785 patch
ActivePublic
Actions

Authored by mcorteel on Feb 1 2018, 10:58 AM.
Tags
None
Referenced Files
F5406484: T12785 patch
Feb 1 2018, 10:58 AM
Subscribers
DragonBe
• shekharsharma705
diff --git a/src/unit/engine/PhpunitTestEngine.php b/src/unit/engine/PhpunitTestEngine.php
index 8206b787..ef01fda4 100644
--- a/src/unit/engine/PhpunitTestEngine.php
+++ b/src/unit/engine/PhpunitTestEngine.php
@@ -52,7 +52,7 @@ final class PhpunitTestEngine extends ArcanistUnitTestEngine {
if (!Filesystem::pathExists($test_path)) {
continue;
}
- $json_tmp = new TempFile();
+ $xml_tmp = new TempFile();
$clover_tmp = null;
$clover = null;
if ($this->getEnableCoverage() !== false) {
@@ -64,10 +64,10 @@ final class PhpunitTestEngine extends ArcanistUnitTestEngine {
$stderr = '-d display_errors=stderr';
- $futures[$test_path] = new ExecFuture('%C %C %C --log-json %s %C %s',
- $this->phpunitBinary, $config, $stderr, $json_tmp, $clover, $test_path);
+ $futures[$test_path] = new ExecFuture('%C %C %C --log-junit %s %C %s',
+ $this->phpunitBinary, $config, $stderr, $xml_tmp, $clover, $test_path);
$tmpfiles[$test_path] = array(
- 'json' => $json_tmp,
+ 'xml' => $xml_tmp,
'clover' => $clover_tmp,
);
}
@@ -81,7 +81,7 @@ final class PhpunitTestEngine extends ArcanistUnitTestEngine {
$results[] = $this->parseTestResults(
$test,
- $tmpfiles[$test]['json'],
+ $tmpfiles[$test]['xml'],
$tmpfiles[$test]['clover'],
$stderr);
}
@@ -90,17 +90,17 @@ final class PhpunitTestEngine extends ArcanistUnitTestEngine {
}
/**
- * Parse test results from phpunit json report.
+ * Parse test results from phpunit XML report.
*
* @param string $path Path to test
- * @param string $json_tmp Path to phpunit json report
+ * @param string $xml_tmp Path to phpunit XML report
* @param string $clover_tmp Path to phpunit clover report
* @param string $stderr Data written to stderr
*
* @return array
*/
- private function parseTestResults($path, $json_tmp, $clover_tmp, $stderr) {
- $test_results = Filesystem::readFile($json_tmp);
+ private function parseTestResults($path, $xml_tmp, $clover_tmp, $stderr) {
+ $test_results = Filesystem::readFile($xml_tmp);
return id(new ArcanistPhpunitTestResultParser())
->setEnableCoverage($this->getEnableCoverage())
->setProjectRoot($this->projectRoot)
diff --git a/src/unit/parser/ArcanistPhpunitTestResultParser.php b/src/unit/parser/ArcanistPhpunitTestResultParser.php
index 5ccff970..3d7fcd77 100644
--- a/src/unit/parser/ArcanistPhpunitTestResultParser.php
+++ b/src/unit/parser/ArcanistPhpunitTestResultParser.php
@@ -9,10 +9,10 @@
final class ArcanistPhpunitTestResultParser extends ArcanistTestResultParser {
/**
- * Parse test results from phpunit json report
+ * Parse test results from phpunit XML report
*
* @param string $path Path to test
- * @param string $test_results String containing phpunit json report
+ * @param string $test_results String containing phpunit XML report
*
* @return array
*/
@@ -25,7 +25,7 @@ final class ArcanistPhpunitTestResultParser extends ArcanistTestResultParser {
return array($result);
}
- $report = $this->getJsonReport($test_results);
+ $report = simplexml_load_string($test_results);
// coverage is for all testcases in the executed $path
$coverage = array();
@@ -36,56 +36,36 @@ final class ArcanistPhpunitTestResultParser extends ArcanistTestResultParser {
$last_test_finished = true;
$results = array();
- foreach ($report as $event) {
- switch (idx($event, 'event')) {
- case 'test':
- break;
- case 'testStart':
- $last_test_finished = false;
- // fall through
- default:
- continue 2; // switch + loop
- }
-
+ foreach ($report->testsuite as $test_suite) {
$status = ArcanistUnitTestResult::RESULT_PASS;
$user_data = '';
- if ('fail' == idx($event, 'status')) {
+ if ((int)$test_suite['failures'] > 0) {
$status = ArcanistUnitTestResult::RESULT_FAIL;
- $user_data .= idx($event, 'message')."\n";
- foreach (idx($event, 'trace') as $trace) {
- $user_data .= sprintf(
- "\n%s:%s",
- idx($trace, 'file'),
- idx($trace, 'line'));
+ foreach ($test_suite->testcase as $test_case) {
+ foreach ($test_case->failure as $failure) {
+ $user_data .= sprintf(
+ "\n%s",
+ (string)$failure);
+ }
}
- } else if ('error' == idx($event, 'status')) {
- if (strpos(idx($event, 'message'), 'Skipped Test') !== false) {
- $status = ArcanistUnitTestResult::RESULT_SKIP;
- $user_data .= idx($event, 'message');
- } else if (strpos(
- idx($event, 'message'),
- 'Incomplete Test') !== false) {
- $status = ArcanistUnitTestResult::RESULT_SKIP;
- $user_data .= idx($event, 'message');
- } else {
- $status = ArcanistUnitTestResult::RESULT_BROKEN;
- $user_data .= idx($event, 'message');
- foreach (idx($event, 'trace') as $trace) {
- $user_data .= sprintf(
- "\n%s:%s",
- idx($trace, 'file'),
- idx($trace, 'line'));
+ } else if ($test_suite['errors'] > 0) {
+ $status = ArcanistUnitTestResult::RESULT_BROKEN;
+ foreach ($test_suite->testcase as $test_case) {
+ foreach ($test_case->error as $error) {
+ $user_data .= sprintf(
+ "\n%s",
+ (string)$error);
}
}
}
- $name = preg_replace('/ \(.*\)/s', '', idx($event, 'test'));
+ $name = preg_replace('/ \(.*\)/s', '', $test_suite['name']);
$result = new ArcanistUnitTestResult();
$result->setName($name);
$result->setResult($status);
- $result->setDuration(idx($event, 'time'));
+ $result->setDuration((float)$test_suite['time']);
$result->setCoverage($coverage);
$result->setUserData($user_data);
@@ -95,7 +75,7 @@ final class ArcanistPhpunitTestResultParser extends ArcanistTestResultParser {
if (!$last_test_finished) {
$results[] = id(new ArcanistUnitTestResult())
- ->setName(idx($event, 'test')) // use last event
+ ->setName($test_suite['name']) // use last event
->setUserData($this->stderr)
->setResult(ArcanistUnitTestResult::RESULT_BROKEN);
}
@@ -161,28 +141,4 @@ final class ArcanistPhpunitTestResultParser extends ArcanistTestResultParser {
return $reports;
}
- /**
- * We need this non-sense to make json generated by phpunit
- * valid.
- *
- * @param string $json String containing JSON report
- * @return array JSON decoded array
- */
- private function getJsonReport($json) {
-
- if (empty($json)) {
- throw new Exception(
- pht(
- 'JSON report file is empty, it probably means that phpunit '.
- 'failed to run tests. Try running %s with %s option and then run '.
- 'generated phpunit command yourself, you might get the answer.',
- 'arc unit',
- '--trace'));
- }
-
- $json = preg_replace('/}{\s*"/', '},{"', $json);
- $json = '['.$json.']';
- return phutil_json_decode($json);
- }
-
}

Event Timeline

mcorteel created this paste.Feb 1 2018, 10:58 AM
mcorteel mentioned this in T12785: Arc unit command fails with PHPUnit 6.
• shekharsharma705 added a subscriber: • shekharsharma705.Feb 9 2018, 10:45 AM

Will this be promoted to stable release of Phabricator?

mcorteel added a comment.Feb 21 2018, 4:18 PM

Once I get an answer on how to submit it... See T12785

DragonBe added subscribers: epriestley, DragonBe.Jun 7 2018, 1:35 PM

Hey @mcorteel, great work on the fix! 🏅

I've tested it myself on several projects that have been upgraded to PHPUnit 7.2.2 and it works like a charm! Unfortunately I don't have any projects with PHPUnit 5 or less, so if someone can test backwards compatibility?

Any feedback from the @epriestley on how you can push your T12785 fix to core?

epriestley removed a subscriber: epriestley.Jun 7 2018, 1:35 PM
mcorteel added a comment.Jun 29 2018, 10:54 AM

@DragonBe Backwards compatibility is broken by this fix. I'm not sure what the phabricator policy is regarding this, but I see two options:

  • Creating another test engine (one for PHPUnit <= 5 and one for PHPUnit > 5)
  • Parsing output differently depending on the PHPUnit version and reuse the old code where needed. This would require a little work to do things properly I guess.
DragonBe added a comment.Jun 29 2018, 11:40 AM

Tough decision to make. Given the fact that most PHP projects are abandoning PHP 5 support is great, but with forcing users to upgrade is not always a good thing, as I'm not sure how this would impact the corporate/enterprise customers.

I do see T7408 is talking about dropping support for older versions, but they're only talking about dropping support for PHP 5.2. That means that 5.3, 5.4, 5.5 and 5.6 still require some form of support. It would be great if there would be some usage stats to give us some insights in the actual platforms running Phabricator.

mcorteel added a comment.Jul 2 2018, 8:39 AM

I didn't even suggest dropping support, I'm merely listing the options to allow both versions to work.
We use a manually patched version of arcanist because it suits our company's use case, but it wouldn't be suitable for a merge in Phabricator's codebase in this state.

DragonBe added a comment.Jul 2 2018, 9:03 AM

I really haven't looked into the backwards compatibility issue as we're already test-driving on PHP 7.3 preview, so I can't help you out there. Your suggestion to detect the PHP version and offer this solution only for versions of PHP 7 and higher seems like the most obvious approach.

Phabricator · Built by Phacility