Page MenuHomePhabricator

Improve UX and general awareness of "remote diff" processing
Open, Needs TriagePublic

Description

Our developers generally don't run their own tests locally, and instead invoke arc diff --nounit, then supply an excuse (which is invariably something along the lines of "lol" or "no" or "tests will run remotely") for why they aren't running them. This is a slight annoyance, but it's later compounded on the diff display where they see this:

Which is not exactly true, and confusing, because after harbormaster has finished building their diff, it attaches the unit test results to it.

Simple actions:

  • Make it possible to run --nounit by default without providing an excuse.
  • Don't indicate to users that the diff is "untested" when it is in fact tested.

Potentially complicated things:

  • Formalize this workflow somehow.

Event Timeline

eadler added a subscriber: eadler.May 23 2016, 10:04 PM
This comment was removed by eadler.

If developers generally don't run tests locally, is having arc unit do anything really valuable in your environment? That is, if you solved this by de-configuring arc so that arc unit did nothing, what would you lose?

(One possible thing you might lose is a consistent way for Harbormaster to execute tests in any project, but I'm not sure if you're using arc unit --target and, if you are, a possible solution might be to add an "only run tests when running in target mode" sort of flag rather than trying to change the behavior of --nounit.)

Yes we use arc unit as the chokepoint for "consistently run unit tests as far as phabricator is concerned", and use the --target flag when harbormaster is running the tests.

Developers do run the test suite locally if they suspect they probably broke things though, it's not "never", it's just "infrequently".

epriestley added a comment.EditedMay 24 2016, 2:31 PM

My general line of thinking is that providing a better idea of a "context" to arc unit (e.g., diff vs build) and letting it make a default decision about which tests to run (possibly including no tests) is probably the most promising way forward here. You could still override this context with some kind of arc diff --run-some-extra-tests flag. I'm not sure if hard-coding a small number of contexts or making them fully user-configurable or something in between is the best way forward. This is probably a longer-term thing.

Also in the mediumer-term you'll likely be able to disable this message through configuration with T6030.

In the short term, this warning message ("Unit tests were skipped...") currently examines only the arc unit tests, but we can make it examine arc + Harbormaster instead. Then it will say "skipped" only if there are no server-side tests. Additionally, it can accurately warn about Harbormaster build failures and caution users if builds are ongoing.

<?php

final class PhavNoUnitCustomArgument extends ICCustomArcanistArgument {

  public function supportsCommand($command) {
    return $command === 'diff';
  }

  public function getConfigurationKey() {
    return 'remote-unit';
  }

  public function getArguments() {
    return [
      'unit' => [
        'help' => pht(
          'Run the unit tests locally and attach the results to the diff.'),
      ],
    ];
  }

  public function willRunWorkflow($command, ArcanistWorkflow $workflow) {
    $passed = $workflow->getPassedArguments();
    if (in_array('--unit', $passed)) {
      return;
    }
    $nounit_arguments = [
      '--nounit',
      '--excuse',
      'remote tests',
    ] + $passed;
    $workflow->parseArguments($nounit_arguments);
  }

}

This works for client side stuff, our "custom argument class" just passes on the word from our custom ArcanistConfiguration.

eadler added a project: Restricted Project.Sep 29 2016, 6:31 PM