| jessjohnson | |
| Apr 20 2017, 8:35 PM |
| F4917570: Screen Shot 2017-04-20 at 3.37.49 PM.png | |
| Apr 20 2017, 10:46 PM |
| F4917568: Screen Shot 2017-04-20 at 3.37.38 PM.png | |
| Apr 20 2017, 10:46 PM |
Overall problem: Engineers are pushing code that fails unit tests or does not build
When unit tests fail or contain a explanation, we want to gate or prevent arc land from committing to a remote repository. Also, if the build has failed (as an example, a failed message received from the CI server), then developers should not be able to land changes.
Given the above restrictions, if a developer really want to land changes they can force and provide an explanation.
Can you provide some more context about why engineers are hitting the "builds failed" prompt, and continuing through it anyway with failing tests and/or nonbuilding code?
If they're hitting "y" now to land broken code, why won't they alias y git commit --amend -m '--force this is important' and just step around a slightly higher barrier if the tools impose one?
Are there other problems (like flaky tests, the warning meaning too many different things, lack of granularity in result reporting, political/social issues we could subvert, etc) we could solve instead?
Engineers are not hitting a prompt when landing, they are disregarding the unit test failure message or not reviewing why the build failed. Since the build failure is on an external system, the engineer needs to view this information separately (the system posts a comment why the build failed). The overall goal of a herald rule blocking the land is so that owner can communicate that checking in code without running or successful unit tests is not cool.
Generally speaking, the owner of the repo maintains good unit tests and does not want any new code that breaks the tests. I am aware that the engineer can just push the code changes outside of Phabricator, but there are some social repercussions of doing that and it happens pretty rarely.
If you integrate with Harbormaster, users will be prompted explicitly when they try to arc land a revision with build failures:
The text looks like this:
Harbormaster failed to build the active diff for this revision. Build failures:
FAILED Build 52326: Jenkins Build
You can review build details here:
Harbormaster URI: https://phabricator.yourcompany.com/B52279
Land revision anyway, despite build failures? [y/N] y...except that the whole thing is mostly red.
Would that solve your problem?
Yes, it would. We haven't fully integrated the build system with Phabricator, but I'll review the docs and see how much more work we need to do.
Unit tests run on the client side as part of an arc extension and there is an explanation that's needed to arc diff those, but it's fairly passive and not gating. I suppose we'll need to run those on the server side and gate as if the build failed (e.g. if unit tests, linting, or the build fails, then we report that the build failed).
Cool. Harbormaster integration will also give you other UI hints like these, to help communicate build status to reviewers so they don't accept a change which has failed or in-progress builds:
Sufficiently recent versions of arc/Phabricator should be able to drive this UI purely from arc unit results -- without needing to fully hook up external build systems to Harbormaster -- but you may be out of date or your extension may be interacting oddly with things. Beyond this, hooking up external build systems can provide richer results.
The expected behavior with modern code is that arc unit failures are sufficient (without further integration work) to cause a prompt on arc land. T9937 reports another install encountering this behavior and I believe it is working as intended. If not, feel free to submit a bug report after updating and disabling extension code.