Page MenuHomePhabricator

Make code coverage optional by default
Closed, ResolvedPublic

Description

I have a custom ArcanistUnitTestEngine implementation, but I believe that this issue is generic. Currently, my test engine contains the following code:

public function run() {
  if ($this->getEnableCoverage() !== false && !extension_loaded('xdebug')) {
    throw new ArcanistUsageException(
      pht(
        'You specified `%s` but %s is not available, so coverage can not '.
        'be enabled for `%s`.',
        '--coverage',
        'XDebug',
        __CLASS__));
  }

If I don't have XDebug installed then running arc unit will hard fail (throw an exception), even without the --coverage flag.

> arc unit -- test/SomeTest.php
Usage Exception: You specified `--coverage` but XDebug is not available, so coverage can not be enabled for `FreelancerPhpunitTestEngine`.

I feel that code coverage should just be silently disabled unless the --coverage flag is explicitly passed.

Event Timeline

getEnableCoverage() returns three values:

  • false: User passed --no-coverage, explicitly disabling coverage.
  • null: User did not pass any coverage flags. Coverage should generally be enabled if available.
  • true: User passed --coverage.

You can use code like the block in PhutilUnitTestEngine to respond to these values:

$enable_coverage = $this->getEnableCoverage();

if ($enable_coverage !== false) {
  if (!function_exists('xdebug_start_code_coverage')) {
    if ($enable_coverage === true) {
      throw new ArcanistUsageException(
        pht(
          'You specified %s but %s is not available, so '.
          'coverage can not be enabled for %s.',
          '--coverage',
          'XDebug',
          __CLASS__));
    }
  } else {
    $enable_coverage = true;
  }
}

In particular, PhutilUnitTestEngine's behavior is:

  • --no-coverage: No coverage.
  • --coverage: Coverage, or error if xdebug is not available.
  • No flag: Coverage if xdebug is available, no coverage otherwise.

I think this is a reasonable default, although you could interpret no flag to mean "no coverage, even if coverage is possible" if you prefer. Does that resolve your issue?

joshuaspence claimed this task.

Yep, that works. I didn't realize that null was a possible return value.