Differential D14495

Prevent double rendering of unit test results
ClosedPublic
Actions

Authored by joshuaspence on Nov 16 2015, 7:48 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

rARCa4dba24c468c: Prevent double rendering of unit test results

Summary

D14037 had the logic slightly wrong and unit test results are now being double rendered.

Test Plan

Ran a unit test with arc unit -- path/to/test and saw the results rendered only once.

Diff Detail

Repository

rARC Arcanist

Branch

master

Lint

Lint OK

Unit

Unit Tests OK

Build Status

Buildable 8878
Build 10387: Run Core Tests
Build 10386: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	11 ms	ArcanistDynamicDefineXHPASTLinterRuleTestCase::Unknown Unit Message ("")
	11 ms	ArcanistPHPCloseTagXHPASTLinterRuleTestCase::Unknown Unit Message ("")
	13 ms	ArcanistAliasFunctionXHPASTLinterRuleTestCase::Unknown Unit Message ("")
	10 ms	ArcanistArrayCombineXHPASTLinterRuleTestCase::Unknown Unit Message ("")
	15 ms	ArcanistArrayIndexSpacingXHPASTLinterRuleTestCase::Unknown Unit Message ("")
		View Full Test Results (2 Failed · 153 Passed · 35 Skipped)

Event Timeline

joshuaspence updated this revision to Diff 35068.Nov 16 2015, 7:48 PM

joshuaspence retitled this revision from to Prevent double rendering of unit test results.

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptNov 16 2015, 7:48 PM

Herald added a subscriber: Korvin. · View Herald Transcript

Maybe add a comment explaining why !$thing causes $thing to occur.

This revision is now accepted and ready to land.Nov 16 2015, 8:52 PM

Closed by commit rARCa4dba24c468c: Prevent double rendering of unit test results (authored by joshuaspence). · Explain WhyNov 17 2015, 8:20 PM

This revision was automatically updated to reflect the committed changes.

@epriestley's comment shows why shouldEchoTestResults is a bad name for this method. A name like testEngineWillEchoTestResults is also awful, but it better communicates the purpose. The problem is that there is no subject in the name. It would be clearer if the method were rendererShouldEchoTestResults and the logic were inverted.

Firehed mentioned this in D15880: Move 'should render' logic to PhutilUnitTestEngine.May 10 2016, 7:09 PM

urzds mentioned this in T11479: ArcanistUnitTestEngine->shouldEchoTestResults() logic inverted.Aug 15 2016, 9:02 PM