diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -62,6 +62,7 @@ 'ArcanistConfigurationDrivenUnitTestEngine' => 'unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php', 'ArcanistConfigurationManager' => 'configuration/ArcanistConfigurationManager.php', 'ArcanistConsoleLintRenderer' => 'lint/renderer/ArcanistConsoleLintRenderer.php', + 'ArcanistConsoleUnitRenderer' => 'unit/renderer/ArcanistConsoleUnitRenderer.php', 'ArcanistConstructorParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php', 'ArcanistControlStatementSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistControlStatementSpacingXHPASTLinterRule.php', 'ArcanistCoverWorkflow' => 'workflow/ArcanistCoverWorkflow.php', @@ -142,6 +143,7 @@ 'ArcanistJSONLintRenderer' => 'lint/renderer/ArcanistJSONLintRenderer.php', 'ArcanistJSONLinter' => 'lint/linter/ArcanistJSONLinter.php', 'ArcanistJSONLinterTestCase' => 'lint/linter/__tests__/ArcanistJSONLinterTestCase.php', + 'ArcanistJSONUnitRenderer' => 'unit/renderer/ArcanistJSONUnitRenderer.php', 'ArcanistJscsLinter' => 'lint/linter/ArcanistJscsLinter.php', 'ArcanistJscsLinterTestCase' => 'lint/linter/__tests__/ArcanistJscsLinterTestCase.php', 'ArcanistKeywordCasingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistKeywordCasingXHPASTLinterRule.php', @@ -182,6 +184,7 @@ 'ArcanistNoLintLinterTestCase' => 'lint/linter/__tests__/ArcanistNoLintLinterTestCase.php', 'ArcanistNoParentScopeXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistNoParentScopeXHPASTLinterRule.php', 'ArcanistNoneLintRenderer' => 'lint/renderer/ArcanistNoneLintRenderer.php', + 'ArcanistNoneUnitRenderer' => 'unit/renderer/ArcanistNoneUnitRenderer.php', 'ArcanistObjectOperatorSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistObjectOperatorSpacingXHPASTLinterRule.php', 'ArcanistPEP8Linter' => 'lint/linter/ArcanistPEP8Linter.php', 'ArcanistPEP8LinterTestCase' => 'lint/linter/__tests__/ArcanistPEP8LinterTestCase.php', @@ -249,11 +252,11 @@ 'ArcanistTodoCommentXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistTodoCommentXHPASTLinterRule.php', 'ArcanistTodoWorkflow' => 'workflow/ArcanistTodoWorkflow.php', 'ArcanistUSEnglishTranslation' => 'internationalization/ArcanistUSEnglishTranslation.php', + 'ArcanistUglyJSONUnitRenderer' => 'unit/renderer/ArcanistUglyJSONUnitRenderer.php', 'ArcanistUnableToParseXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnableToParseXHPASTLinterRule.php', 'ArcanistUnaryPostfixExpressionSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnaryPostfixExpressionSpacingXHPASTLinterRule.php', 'ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule.php', 'ArcanistUndeclaredVariableXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php', - 'ArcanistUnitConsoleRenderer' => 'unit/renderer/ArcanistUnitConsoleRenderer.php', 'ArcanistUnitRenderer' => 'unit/renderer/ArcanistUnitRenderer.php', 'ArcanistUnitTestEngine' => 'unit/engine/ArcanistUnitTestEngine.php', 'ArcanistUnitTestResult' => 'unit/ArcanistUnitTestResult.php', @@ -351,6 +354,7 @@ 'ArcanistConfigurationDrivenUnitTestEngine' => 'ArcanistUnitTestEngine', 'ArcanistConfigurationManager' => 'Phobject', 'ArcanistConsoleLintRenderer' => 'ArcanistLintRenderer', + 'ArcanistConsoleUnitRenderer' => 'ArcanistUnitRenderer', 'ArcanistConstructorParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistControlStatementSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistCoverWorkflow' => 'ArcanistWorkflow', @@ -431,6 +435,7 @@ 'ArcanistJSONLintRenderer' => 'ArcanistLintRenderer', 'ArcanistJSONLinter' => 'ArcanistLinter', 'ArcanistJSONLinterTestCase' => 'ArcanistLinterTestCase', + 'ArcanistJSONUnitRenderer' => 'ArcanistUnitRenderer', 'ArcanistJscsLinter' => 'ArcanistExternalLinter', 'ArcanistJscsLinterTestCase' => 'ArcanistExternalLinterTestCase', 'ArcanistKeywordCasingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', @@ -471,6 +476,7 @@ 'ArcanistNoLintLinterTestCase' => 'ArcanistLinterTestCase', 'ArcanistNoParentScopeXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistNoneLintRenderer' => 'ArcanistLintRenderer', + 'ArcanistNoneUnitRenderer' => 'ArcanistUnitRenderer', 'ArcanistObjectOperatorSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistPEP8Linter' => 'ArcanistExternalLinter', 'ArcanistPEP8LinterTestCase' => 'ArcanistExternalLinterTestCase', @@ -538,11 +544,11 @@ 'ArcanistTodoCommentXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistTodoWorkflow' => 'ArcanistWorkflow', 'ArcanistUSEnglishTranslation' => 'PhutilTranslation', + 'ArcanistUglyJSONUnitRenderer' => 'ArcanistUnitRenderer', 'ArcanistUnableToParseXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistUnaryPostfixExpressionSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistUndeclaredVariableXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', - 'ArcanistUnitConsoleRenderer' => 'ArcanistUnitRenderer', 'ArcanistUnitRenderer' => 'Phobject', 'ArcanistUnitTestEngine' => 'Phobject', 'ArcanistUnitTestResult' => 'Phobject', diff --git a/src/unit/renderer/ArcanistUnitConsoleRenderer.php b/src/unit/renderer/ArcanistConsoleUnitRenderer.php rename from src/unit/renderer/ArcanistUnitConsoleRenderer.php rename to src/unit/renderer/ArcanistConsoleUnitRenderer.php --- a/src/unit/renderer/ArcanistUnitConsoleRenderer.php +++ b/src/unit/renderer/ArcanistConsoleUnitRenderer.php @@ -1,6 +1,10 @@ getResult(); diff --git a/src/unit/renderer/ArcanistJSONUnitRenderer.php b/src/unit/renderer/ArcanistJSONUnitRenderer.php new file mode 100644 --- /dev/null +++ b/src/unit/renderer/ArcanistJSONUnitRenderer.php @@ -0,0 +1,27 @@ +results, $result); + + return ''; + } + + public function renderPostamble() { + assert_instances_of($this->results, 'ArcanistUnitTestResult'); + $data = array_values(mpull($this->results, 'toDictionary')); + return id(new PhutilJSON())->encodeFormatted($data); + } + +} diff --git a/src/unit/renderer/ArcanistNoneUnitRenderer.php b/src/unit/renderer/ArcanistNoneUnitRenderer.php new file mode 100644 --- /dev/null +++ b/src/unit/renderer/ArcanistNoneUnitRenderer.php @@ -0,0 +1,13 @@ +results, $result); + + return ''; + } + + public function renderPostamble() { + assert_instances_of($this->results, 'ArcanistUnitTestResult'); + $data = array_values(mpull($this->results, 'toDictionary')); + return phutil_json_encode($data); + } + +} diff --git a/src/unit/renderer/ArcanistUnitRenderer.php b/src/unit/renderer/ArcanistUnitRenderer.php --- a/src/unit/renderer/ArcanistUnitRenderer.php +++ b/src/unit/renderer/ArcanistUnitRenderer.php @@ -1,5 +1,33 @@ setAncestorClass(__CLASS__) + ->setUniqueMethod('getId') + ->execute(); + } + + final public static function getRendererById($id) { + $renderers = self::loadAllRenderers(); + if ($renderers[$id]) { + return $renderers[$id]; + } else { + throw new Exception('Renderer, '.$id.', does not exist.'); + } + } + + abstract public function getId(); + + public function renderPreamble() { + return ''; + } + abstract public function renderUnitResult(ArcanistUnitTestResult $result); + + public function renderPostamble() { + return ''; + } + } diff --git a/src/workflow/ArcanistUnitWorkflow.php b/src/workflow/ArcanistUnitWorkflow.php --- a/src/workflow/ArcanistUnitWorkflow.php +++ b/src/workflow/ArcanistUnitWorkflow.php @@ -69,9 +69,6 @@ 'Show a detailed coverage report on the CLI. Implies %s.', '--coverage'), ), - 'json' => array( - 'help' => pht('Report results in JSON format.'), - ), 'output' => array( 'param' => 'format', 'help' => pht( @@ -79,10 +76,6 @@ "With 'json', report results in JSON format. ". "With 'ugly', use uglier (but more efficient) JSON formatting. ". "With 'none', don't print results."), - 'conflicts' => array( - 'json' => pht('Only one output format allowed'), - 'ugly' => pht('Only one output format allowed'), - ), ), 'target' => array( 'param' => 'phid', @@ -96,11 +89,6 @@ 'rev' => pht('%s runs all tests.', '--everything'), ), ), - 'ugly' => array( - 'help' => pht( - 'With %s, use uglier (but more efficient) formatting.', - '--json'), - ), '*' => 'paths', ); } @@ -153,7 +141,9 @@ } $this->engine->setArguments($this->getPassthruArgumentsAsMap('unit')); - $renderer = new ArcanistUnitConsoleRenderer(); + // Get renderer based on '--output' argument + $renderer = ArcanistUnitRenderer::getRendererById($this->getOutputFormat()); + $this->engine->setRenderer($renderer); $enable_coverage = null; // Means "default". @@ -165,7 +155,7 @@ } $this->engine->setEnableCoverage($enable_coverage); - // Enable possible async tests only for 'arc diff' not 'arc unit' + // Enable possible async tests only for 'arc diff' not 'arc unit'. if ($this->getParentWorkflow()) { $this->engine->setEnableAsyncTests(true); } else { @@ -178,24 +168,30 @@ $this->testResults = $results; - $console = PhutilConsole::getConsole(); - - $output_format = $this->getOutputFormat(); - - if ($output_format !== 'full') { - $console->disableOut(); - } - $unresolved = array(); $coverage = array(); + $overall_result = self::RESULT_OKAY; + // Gather unresolved tests and coverage while determining the + // 'overall_result'. + foreach ($results as $result) { $result_code = $result->getResult(); - if ($this->engine->shouldEchoTestResults()) { - $console->writeOut('%s', $renderer->renderUnitResult($result)); - } if ($result_code != ArcanistUnitTestResult::RESULT_PASS) { $unresolved[] = $result; } + + // Note: This means as soon as a RESULT_FAIL/BROKEN/UNSOUND are found, + // they persist as the '$overall_result'. + if ($overall_result != self::RESULT_FAIL) { + $result_code = $result->getResult(); + if ($result_code == ArcanistUnitTestResult::RESULT_FAIL || + $result_code == ArcanistUnitTestResult::RESULT_BROKEN) { + $overall_result = self::RESULT_FAIL; + } else if ($result_code == ArcanistUnitTestResult::RESULT_UNSOUND) { + $overall_result = self::RESULT_UNSOUND; + } + } + if ($result->getCoverage()) { foreach ($result->getCoverage() as $file => $report) { $coverage[$file][] = $report; @@ -203,6 +199,25 @@ } } + $this->unresolvedTests = $unresolved; + + // Set up output methods. + $console = PhutilConsole::getConsole(); + + // Write out Unit Test results to console or file. + if ($this->engine->shouldEchoTestResults()) { + $preamble = $renderer->renderPreamble(); + $console->writeOut('%s', $preamble); + + foreach ($results as $result) { + $console->writeOut('%s', $renderer->renderUnitResult($result)); + } + + $postamble = $renderer->renderPostamble(); + $console->writeOut('%s', $postamble); + } + + // Process and render coverage results. if ($coverage) { $file_coverage = array_fill_keys( $paths, @@ -243,41 +258,6 @@ } } - $this->unresolvedTests = $unresolved; - - $overall_result = self::RESULT_OKAY; - foreach ($results as $result) { - $result_code = $result->getResult(); - if ($result_code == ArcanistUnitTestResult::RESULT_FAIL || - $result_code == ArcanistUnitTestResult::RESULT_BROKEN) { - $overall_result = self::RESULT_FAIL; - break; - } else if ($result_code == ArcanistUnitTestResult::RESULT_UNSOUND) { - $overall_result = self::RESULT_UNSOUND; - } - } - - if ($output_format !== 'full') { - $console->enableOut(); - } - $data = array_values(mpull($results, 'toDictionary')); - switch ($output_format) { - case 'ugly': - $console->writeOut('%s', json_encode($data)); - break; - case 'json': - $json = new PhutilJSON(); - $console->writeOut('%s', $json->encodeFormatted($data)); - break; - case 'full': - // already printed - break; - case 'none': - // do nothing - break; - } - - $target_phid = $this->getArgument('target'); if ($target_phid) { $this->uploadTestResults($target_phid, $overall_result, $results); @@ -336,12 +316,6 @@ } private function getOutputFormat() { - if ($this->getArgument('ugly')) { - return 'ugly'; - } - if ($this->getArgument('json')) { - return 'json'; - } $format = $this->getArgument('output'); $known_formats = array( 'none' => 'none',