Page MenuHomePhabricator

Move 'should render' logic to PhutilUnitTestEngine
AbandonedPublic

Authored by epriestley on May 10 2016, 7:09 PM.

Details

Reviewers
joshuaspence
Firehed
Group Reviewers
Blessed Reviewers
Summary

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)

Test Plan

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

Repository
rARC Arcanist
Branch
fix_logic (branched from master)
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 12121
Build 15283: arc lint + arc unit

Event Timeline

Firehed updated this revision to Diff 38246.May 10 2016, 7:09 PM
Firehed retitled this revision from to Move 'should render' logic to PhutilUnitTestEngine.
Firehed updated this object.
Firehed edited the test plan for this revision. (Show Details)
Firehed added reviewers: epriestley, joshuaspence.
epriestley edited edge metadata.May 10 2016, 7:24 PM

How can I reproduce whatever root issue this causes?

(Presumably, a double render?)

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)

Sorry, I mean: what does this actually fix? What problem does this change solve?

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.

svemir added a subscriber: svemir.
epriestley commandeered this revision.Sep 27 2018, 2:25 PM
epriestley abandoned this revision.
epriestley edited reviewers, added: Firehed; removed: epriestley.

Mooted by T13098.