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.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- rPHU6880c6cc8a8d: Attempt to build XHPAST during unit tests
Deleted src/parser/xhpast/bin/xhpast and ran arc unit -- src/parser/xhpast.
Diff Detail
Diff Detail
- Repository
- rPHU libphutil
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 4230 Build 4243: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
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").
Comment Actions
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(...); } }
Comment Actions
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.