Attempt to build XHPAST during unit tests
ClosedPublic
Actions

Authored by joshuaspence on Jan 27 2015, 7:36 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

rPHU6880c6cc8a8d: Attempt to build XHPAST during unit tests

Summary

If we have an out-of-date XHPAST binary, attempt to rebuild XHPAST as a part of running the unit tests. If XHPAST is missing (or we cannot build it for whatever reason), just skip the unit tests as per the existing behavior.

Test Plan

Deleted src/parser/xhpast/bin/xhpast and ran arc unit -- src/parser/xhpast.

Diff Detail

Repository

rPHU libphutil

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

joshuaspence updated this revision to Diff 27695.Jan 27 2015, 7:36 AM

joshuaspence retitled this revision from to Don't allow XHPAST tests to be skipped.

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJan 27 2015, 7:36 AM

Herald added subscribers: epriestley, Korvin. · View Herald Transcript

joshuaspence mentioned this in D11517: Create a PHP wrapper class for the XHPAST binary.Jan 27 2015, 10:02 AM

joshuaspence updated this object.Jan 27 2015, 10:21 AM

joshuaspence edited edge metadata.

joshuaspence edited the test plan for this revision. (Show Details)

I think we should be liberal about skipping tests.

The reason to skip these tests is that you can't automatically build xhpast and don't want to bother setting up your machine so you can. I think this is a reasonable point of view to have, and comparable to skipping hg or svn tests, and that it's OK to be liberal about skipping tests because we'll eventually fill in the gaps with CI, and, as of today, I think we see very few issues which would have been caught by executing skipped tests.

(In particular, on OSX, you need to install the "Developer Tools" to get GCC, and the first time I did it I had to Google to figure it out, and it's a 2GB download and involves a bunch of extra steps (the most recent version made me agree to a EULA the next time I ran git!) and you need an Apple/iTunes account. Or you need to install brew or something, I guess, and best of luck with that. This isn't a huge huge deal, but it's more involved than apt-get install gcc.)

Overall, I'm hesitant to make installing gcc a prerequisite for Phabricator development, because in reasonable situations it might make writing a first patch (e.g., a typo fix) something like 10x harder than it otherwise would be, and I don't think there's very much benefit to be had in running tests that depend on things you don't have installed. That is, it's fairly hard to break XHPAST with a defensible change on a system that doesn't have XHPAST built. This is slightly less true for hg vs git stuff, which has more overlap, but I think we're seeing a very low overall slop rate here. And this is 100% catchable with CI eventually, which will align the incentives correctly ("Oh, let me go install gcc so I can fix that, I know there's a problem with my code") vs ("There's no possible way I broke xhpast but I still have to install gcc, this sucks").

This revision now requires changes to proceed.Jan 27 2015, 2:17 PM

Right, that's a reasonable point-of-view to have. I suppose that the intention here was to prevent XHPAST tests from being skipped if you can reasonably build XHPAST. The thought was that if you had previously built XHPAST but had just forgot to rebuild it recently, then I don't think that this is a reason to skip the tests.

A slightly more correct approach might be this:

public function testParser() {
  $available = xhpast_is_available();

  if (!$available) {
    try {
      xhpast_build();
    } catch (Exception $e) {
      $this->assertSkipped(...);
    }
  }

That alternate approach (if it was built in the past and is merely out of date, try to rebuild it; then skip the test if it fails) seems reasonable to me.