D14495 changed logic in ArcanistConfigurationDrivenUnitTestEngine in order to prevent double-rendering of test results. However, this logic appears to have been added in the wrong place, as Phutil results self-render. Following the rendering stack, any other engine that supports being ConfigurationDriven would have to render its own results (and have shouldEchoTestResults() return false)
Details
Details
- Reviewers
joshuaspence Firehed - Group Reviewers
Blessed Reviewers
Ran arc unit and saw test results printed only once. Also confirmed that they were double-printed as previously described by applying the change to only one place.
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Branch
- fix_logic (branched from master)
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 12121 Build 15283: arc lint + arc unit
Event Timeline
Comment Actions
After applying the diff locally, in PhutilUnitTestEngine change shouldEchoTestResults to return true (or comment out the addition entirely, since the parent does the same):
12:27:23 firehed@Erics-MacBook-Pro ~/dev/arcanist [0]$ git diff diff --git a/src/unit/engine/PhutilUnitTestEngine.php b/src/unit/engine/PhutilUnitTestEngine.php index cf9401f..2a3ff11 100644 --- a/src/unit/engine/PhutilUnitTestEngine.php +++ b/src/unit/engine/PhutilUnitTestEngine.php @@ -224,6 +224,7 @@ final class PhutilUnitTestEngine extends ArcanistUnitTestEngine { public function shouldEchoTestResults() { // If a renderer exists, PhutilTestCase will automatically render itself; // otherwise, expect an upstream renderer to handle this. + return true; return !$this->renderer; } } 12:27:31 firehed@Erics-MacBook-Pro ~/dev/arcanist [0]$ arc unit PASS 4ms★ PhpunitTestEngineTestCase::testSearchLocations PASS 2ms★ PhutilUnitTestEngineTestCase::testTryTestCases PASS 1ms★ PhutilUnitTestEngineTestCase::testPass PASS 12ms★ PhutilUnitTestEngineTestCase::testGetTestPaths PASS 9ms★ PhutilUnitTestEngineTestCase::testFailSkip PASS 2ms★ PhutilUnitTestEngineTestCase::testTryTestMap PASS 2ms★ ArcanistUnitTestResultTestCase::testCoverageMerges PASS 129ms ArcanistLibraryTestCase::testLibraryMap PASS 151ms ArcanistLibraryTestCase::testEverythingImplemented PASS 1.0s ArcanistLibraryTestCase::testMethodVisibility PASS 4ms★ PhpunitTestEngineTestCase::testSearchLocations PASS 2ms★ PhutilUnitTestEngineTestCase::testTryTestCases PASS 1ms★ PhutilUnitTestEngineTestCase::testPass PASS 12ms★ PhutilUnitTestEngineTestCase::testGetTestPaths PASS 9ms★ PhutilUnitTestEngineTestCase::testFailSkip PASS 2ms★ PhutilUnitTestEngineTestCase::testTryTestMap PASS 2ms★ ArcanistUnitTestResultTestCase::testCoverageMerges PASS 129ms ArcanistLibraryTestCase::testLibraryMap PASS 151ms ArcanistLibraryTestCase::testEverythingImplemented PASS 1.0s ArcanistLibraryTestCase::testMethodVisibility COVERAGE REPORT 83% src/unit/engine/PhutilUnitTestEngine.php
(space added for clarity)
Comment Actions
Ah, right.
I'm trying to get the PHPUnit engine working with .arcunit, like the phutil engine. Without this change, the results will never render unless the engine renders its own results (as phutil currently does). The Arc Unit workflow itself skips rendering the overall results since ArcanistConfigurationDrivenTestEngine::shouldEchoTestResults() returns false, as it contains logic to render results (changed in this diff).
Overall, that's the desired behavior, since you can mix-and-match engines that self-render with those that do not. But because the per-engine rendering logic is backwards, it actually forces the child engines to render rather than allowing them to indicate whether they do.
https://secure.phabricator.com/differential/diff/38247/ is the broader concept.