Page MenuHomePhabricator

ArcanistUnitTestEngine->shouldEchoTestResults() logic inverted
Closed, ResolvedPublic

Description

ArcanistUnitTestEngine.php contains this code:

ArcanistUnitTestEngine.php
/**
 * Modify the return value of this function in the child class, if you do
 * not need to echo the test results after all the tests have been run. This
 * is the case for example when the child class prints the tests results
 * while the tests are running.
 */
public function shouldEchoTestResults() {
  return true;
}

The documentation suggests that true (the setting in the base class) means that "I do need [Arcanist] to echo the test results" and changing it to false would mean "the child class prints the test results itself, Arcanist does not need to print them again".

However, the logic in ArcanistConfigurationDrivenUnitTestEngine.php implements to opposite behaviour:

ArcanistConfigurationDrivenUnitTestEngine.php
foreach ($results as $result) {
  // If the proxied engine renders its own test results then there
  // is no need to render them again here.
  if (!$engine->shouldEchoTestResults()) {
    echo $renderer->renderUnitResult($result);
  }
}

The bug was introduced in D14495. @Firehed appears to have stumbled upon the same issue, which seems to be the reason for them creating D15880.

I suggest that either the name or the logic be changed, so that both will match each other.

Event Timeline

urzds added a subscriber: Firehed.
urzds renamed this task from ArcanistUnitTestEngine.shouldEchoTestResults() logic inverted to ArcanistUnitTestEngine->shouldEchoTestResults() logic inverted.Aug 15 2016, 9:31 PM
epriestley claimed this task.
epriestley added a subscriber: epriestley.

This is effectively fixed by T13098, which modularizes test output.