I don't think there's any way we can reasonably run pre-receive hooks from the test console in the general case. The pre-receive pipeline reads state directly from the underlying VCS, and much of that state is transient (e.g., we may read svn look --transaction <txnid> or whatever). If a user gives us a push log PHID to test later, we can't really rebuild most of the state.

In the simple case of things like bodies and titles we could do something reasonable, but we'd have to go down a completely different code path to do it.

Documenting this seems reasonable, though.

Related, the test console has a big if ($x instanceof ...) mess that should be pushed down into Adapters. A reasonable flow might be:

• Choose an object.
• If only one adapter supports it, do the test run.
• If several adapters support it, let the user choose which rule phase to run.
• If the user chooses a commit and then runs pre-commit rules, just throw an exception explaining that they won't/can't reasonably work (or disable the selection and put the note on the workflow, ideally).

This makes things discoverable without requiring users to dig through documentation.

See also T7961, which is another modular behavior adapters should have.

Actually, that report is already fixed. For completeness, the behavior adapters should have is:

• If an application is not installed, the adapter is not available for rule creation or from the test console.

This may already be the behavior, at least partially, although I suspect the test console does not currently respect it if so.

Actually, that report is already fixed.

Or maybe not!!

First: HeraldTestConsoleController is currently a big mess with a huge instanceof block.

That code should look more like this:

$usable_adapters = array();

$all_adapters = HeraldAdapter::getAllAdapters();
foreach ($all_adapters as $adapter) {
  if ($adapter->supportsTestRunsOnObject($object)) {
    $usable_adapters[] = $adapter;
  }
}

Then move all the ($object instanceof Whatever) stuff into the Adapter subclasses, in new implementations of supportsTestRunsOnObject($object).

You should be able to make this change without affecting the test console's behavior. This will allow future and third-party adapters to interact with the test console automatically without needing to modify the Controller.

Next, add an intermediary screen after the user selects an object but before the test run occurs, where they choose an adapter. This should look something like /herald/new/ does, but offer choices between adapters.

Give adapters some methods so they can render text here, e.g. "Run Revision Update Hooks", "Do a test run of Herald rules as though this revision was just created or updated."

Allow adapters to return true from supportsTestRunsOnObject(), but false from some new method like canReallyDoATestRun(). If they return false, render them in a disabled state on this screen.

Let the pre-commit adapters do test runs, but not really do test runs. Have them render explanatory text when given a revision ("Pre-commit hooks do not support test runs.").

High level goals are:

• Clean up instanceof junk on the test console so a new third-party adapter could work with the test console without upstream changes.
• When a user who is trying to test pre-commit hooks types a commit in to the test console, the next screen should make it clear that those hooks don't support test runs and they won't be able to do simulated tests.

Should we maybe have the dragon also put a link to the relevant herald transcript?

Ah, your underlying issue is difficulty understanding why pushes are rejected?

Can you give me an example of the sort of hook you're using that isn't always clear? The only rule we have on this install emits this message:

The body of this commit is too large. Make a smaller commit.

...which is fairly straightforward.

I don't think linking to the transcript is a bad idea, but maybe there are ways we can make the rule or message itself more clear?

I guess the short of it is, I have rules to allow pushes, instead of rules to block pushes:

• H1 is blocking all commits that have no Revision, except if repo is marked "Hacky"
• H2 is blocking all branches, except those whitelisted by H3
• H3 is a Whitelist, but some conditions are complicated, and are delegated to sub-whitelists:
• H4 is repo's project is "Being Ported" and pusher is Aviv; H5 and H6 are similar for slightly different whitelist conditions.

Haha, okay. Let's just add a link to the transcript.

I've also noticed that Herald Transactions have PHID, but not PHIDType class (https://secure.phabricator.com/diffusion/P/browse/master/src/applications/herald/storage/transcript/HeraldTranscript.php;3f439e25bc586ff8bae203a47625f6c360e4402a$193). Wasn't sure if "completeness" is a good enough reason to make the changes (PHIDType + teach the Query object about withPHIDs, and maybe a bit more).

Yeah, completeness is definitely sufficient motivation if you're up for it.

👍

(I think we're happy with the feature-space here)

T7961 is probably worth fixing while I'm in here.