Page MenuHomePhabricator

Upload coverage to a diff
Closed, ResolvedPublic

Description

I'm trying to integrate coverage into Phabricator, but without using arc. I have code that generates coverage data in the desired format, and I can upload it using conduit/diffusion.updatecoverage. But that updates a commit -- I want to do it for a specific diff. Is there a way to do this? Or would you be willing to add a new conduit endpoint?

Event Timeline

gvanrossum updated the task description. (Show Details)
gvanrossum added a subscriber: gvanrossum.

Yes, you can use harbormaster.sendmessage to report coverage information and unit test results. The API call has fairly thorough documentation on the console page:

https://secure.phabricator.com/conduit/method/harbormaster.sendmessage/

We currently require coverage to be reported per-test, but if you track coverage as a whole you can report a dummy "pass" test result with aggregate coverage.

This document describes the expected workflow for using Harbormaster overall:

https://secure.phabricator.com/book/phabricator/article/harbormaster/

Roughly, the intent is:

  • When a revision is created, a Herald rule runs a build plan in Harbormaster.
  • The build plan asks the external build system (here, your coverage runner) to build the revision.
  • The external system builds the revision, then calls harbormaster.sendmessage to report results (including optional lint, unit, and coverage information).

Does that workflow fit into your approach?

See also:

  • Some discussion about aggregate vs per-test coverage in T9365.
  • Some philosophical discussion about coverage in T9524.

Specifically, I think your ultimate call parameters should look something like this, if you are purely reporting coverage and not test results:

{
  "buildTargetPHID": "the-phid-from-the-call-made-to-your-build-system",
  "type": "pass",
  "unit": [
    "name": "Aggregate Coverage Information",
    "result": "pass",
    "coverage": {
      "path/to/file/A.py": "UUUUNNNCNCNNCNUNUNUCNCXXNNXN"
    }
  ]
}
gvanrossum claimed this task.

Yes, thanks! That flow looks like it will work.

Two follow-up questions, as I'm stuck again, although further down the road:

(a) There doesn't seem to be a hardbormaster query for build targets. I would need this to map a revision/diff to a build target phid, which I need for the harbormaster.sendmessage call. It's supposed to be sent with the call to the external build system, but that doesn't seem to work either -- the build target phid received is always ''.

(b) Is there a way to retrieve coverage from Phabricator once it is stored? I have a need to merge additional coverage (rather than overwriting the existing coverage).

It's supposed to be sent with the call to the external build system, but that doesn't seem to work either -- the build target phid received is always ''.

The expectation is that it will be sent in the call to the build system if you configure it as a query parameter, and I can't immediately reproduce this issue. Here's what I did to try to reproduce it:

First, I created a simple build plan with one "Make HTTP Request" build step that submits to a "requestb.in" URL. Note the URL and the inclusion of ${build.target} as a query parameter:

step.png (656×1 px, 96 KB)

I ran the plan manually, entering any valid object (I used D68) locally:

step2.png (837×1 px, 130 KB)

I waited a minute and reloaded the page, and saw a successful "build":

step3.png (837×1 px, 131 KB)

I clicked the step name ("Make HTTP Request") to look at the details, and saw a successful request / response from the Harbormaster side:

step4.png (837×1 px, 154 KB)

Then I looked at the "requestb.in" target page, and saw that it had received the target PHID:

step6.png (1×1 px, 140 KB)

I confirmed that this was the correct target PHID for the target I'd just created:

step7.png (837×1 px, 129 KB)

My best guess is that maybe you aren't including the variable in your request in the step configuration? We don't submit an HTTP variable named "target.phid" by default, because:

  • you may want to map it to a different parameter, put it as a path component, etc.;
  • most build steps don't care about most variables; and
  • although there is a default way to pass variables for HTTP requests, there isn't for other types of build steps like "run (CLI) command".

If this is the case, maybe we could make the instructions more clear. If this isn't the case, I'm not immediately sure what's going wrong. Can you reproduce the issue with a site like "requestb.in" as the target?

I have a need to merge additional coverage (rather than overwriting the existing coverage).

The expectation is that we should automatically merge all the coverage you submit, so it should be sufficient to just keep writing more coverage as you generate it and let Phabricator handle merging it. A highly granular test setup may submit individual coverage profiles for each unit test (we do this with the upstream integration), and retaining this information is at least theoretically valuable (it allows us to show you which tests cover a line).

Here's what I did to verify this works:

I picked an arbitrary revision with no coverage information but which some Harbormaster activity had run against. This just makes it easier to find a valid Build Target PHID to submit coverage to:

Screen Shot 2015-10-14 at 3.16.12 AM.png (611×1 px, 38 KB)

I followed the links under "Build Status" for this diff to find any valid build target PHID, since I'm just going to dump coverage into it and don't really care what it was originally. In my case, I found PHID-HMBT-sewdi63mqjbl4juonnas under the "Variables" tab of a step.

I made a call to harbormaster.sendmessage via the web console with this payload as "unit":

[
  {
    "name": "Phase 1 Coverage",
    "result": "pass",
    "coverage": {
      "alphabet": "UUUUCNCUUUUUUUXX"
    }
  }
]

Overall, the console looked like this before I submitted the request:

Screen Shot 2015-10-14 at 3.22.02 AM.png (482×1 px, 61 KB)

After I submitted the request, the coverage appeared on the revision:

Screen Shot 2015-10-14 at 3.22.52 AM.png (604×1 px, 37 KB)

I followed the same steps again, but this time I submitted the coverage string CCCC. Here's how merging works:

Coverage AUUUUCNCUUUUUUUXX
Coverage BCCCC
Merged CoverageCCCCCNCUUUUUUUXX

Broadly, if any coverage report has a "C" in a given position, the merged result has a "C" in that position. For other positions, we choose a value from any coverage report with a value arbitrarily (the expectation is that all coverage reports will agree on how many lines the file has, which lines are not executable, etc).

This is the exact algorithm:

https://secure.phabricator.com/diffusion/ARC/browse/master/src/unit/ArcanistUnitTestResult.php$124

After I submitted the second request, Differential reflected the additional coverage. Note that lines 1-4 now show blue ("C"overed), while lines 5-16 still show the original information (since I didn't add any new information):

Screen Shot 2015-10-14 at 3.25.56 AM.png (600×1 px, 37 KB)

Let me know if that makes sense or where you're running into trouble?

Thanks -- it's hugely helpful to know that Phabricator already merges coverage!

Regarding the built target PHID, our code for calling Changes from Phabricator is here:
https://github.com/dropbox/phabricator-changes/blob/master/support/applications/changes/helper/ChangesBuildHelper.php#L127

AFAICT the 'phabricator.buildTargetPHID' parameter passed to Changes is always empty. I'm guessing this is because executeBuild() is called with one parameter (https://github.com/dropbox/phabricator-changes/blob/master/support/applications/changes/herald/ChangesNotifyCustomAction.php#L54) -- what can I do at the call site to fill in the right value?

Aha!

It looks like that code isn't interacting with Harbormaster at all and is making an HTTP request to Changes directly from inside a Herald action. We (fairly strongly) discourage this as a way to communicate with external systems (see T5462 for discussion/rationale -- it's synchronous and blocks the UI, error handling can't happen properly, and there's no way to retry if it fails). There is no build target available during the execution of Herald actions and I can't really come up with a reasonable way to generate one.

The preferred way to implement this action is to have it create a HarbormasterBuildRequest describing the plan you want to run (something like "Make an HTTP request to Changes") with any extra parameters, and queue it for evaluation with $adapter->queueHarbormasterBuildRequest($request). This will execute the plan after Herald exits, generate a build and appropriate build targets, then run the steps normally from within Harbormaster. T9352 has an example use case and P1860 is an example action using this approach (HarbormasterRunBuildPlansHeraldAction is the generic upstream implementation and might also be worth looking at if you want to pursue this).

However, this change is complex and may not be straightforward since it looks like the integration also has a lot of other magic going on which would be affected. I imagine much of it was working around missing capabilities at the time it was written, but we now have many of those capabilities in the upstream and some of the approaches (like POST'ing a raw patch) are discouraged (see "Change Handoff" in the Harbormaster User Guide) or were never encouraged (like doing any of this from inside Herald).

One possible alternative is to add an "Autoplan", by copy/pasting HarbormasterBuildArcanistAutoplan and renaming things. You'll also need to create an automatic step for the plan (like HarbormasterArcLintBuildStepImplementation). Broadly, "Autoplans" allow you to create implicit plans that let you push stuff into Harbormaster and create build targets automatically. This approach is piling more hacks on the pile of existing hacks, but probably has a shorter path to a functional outcome.

After defining the autoplans, use harbormaster.queryautotargets to get the Build Target PHID for your implicit target. The way this method works is by returning the target PHID if it already exists, or implicitly creating it if it does not.

You can actually skip more than that, and just look for one of the existing autoplans, e.g.: call harbormaster.queryautotargets with targetKeys=["arcanist.unit"]. This will give you the "Arcanist Unit Results" target PHID, or create it if it does not exist. This is vaguely misleading, but is the shortest path forward from a purely functional point of view of getting coverage to show up.

We can also help you rewrite the integration in a modern way, but this is firmly in the realm of custom development (which we charge an unreasonable rate for), and probably quite a bit of work because I'd need to configure Changes locally to be able to test things properly. I mention this mostly for completeness -- I think just reusing the Arcanist autotarget is probably the most attractive option here as it should be a couple of lines of code and get you 99% of what you want without really putting the whole system in a worse position.

Thanks, in the last option, what kind of thing should I pass into harbormaster.queryautotargets for the objectPHID? If I leave it blank I get

{"result":null,"error_code":"ERR-CONDUIT-CORE","error_info":"No such object \"\" exists."}

My request was

{
  "targetKeys": [
    "arcanist.unit"
  ]
}

Try the diff PHID -- $diff->getPHID()?

Yes, that works! It looks like it takes an object of one of these forms PHID-DREV-xxxx (a revision), PHID-DIFF-xxxx (a specific diff in that revision), or a PHID-HMBB-xxxx (a buildable).

I'll play around with this some more but it looks like I have a working hack for now. Thanks again!

angie added a project: Restricted Project.Oct 23 2015, 7:28 PM
angie moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.
eadler added a project: Restricted Project.Aug 4 2016, 6:47 PM