Two parts to this:

• The UI shows skipped. Instead, it should just be omitted or show tailored messages that this is an update from the VCS. This is easy and just a display issue.
• We don't re-run lint and unit tests between merging and landing. This is complicated and messy.

The UI shows skipped. Instead, it should just be omitted or show tailored messages that this is an update from the VCS. This is easy and just a display issue.

Agreed. Still, event showing that (omitted) creates the illusion of the diff done without linting or unit tests all the way through since that is the first thing you see unless you dig deeper.

We don't re-run lint and unit tests between merging and landing. This is complicated and messy.

Can't we run arc unit and arc lint right before pushing and if they fail clean up by doing git reset --hard HEAD^1 and switching to the branch? Obviously I'm only thinking about the git case here where arc needs to handle hg and svn too but I'd like to hear more about this since I've landed code which actually broke unit tests in the past during a merge or rebase. It was not fun :)

In T5064#59055, @BYK wrote:

Can't we run arc unit and arc lint right before pushing and if they fail clean up by doing git reset --hard HEAD^1 and switching to the branch? Obviously I'm only thinking about the git case here where arc needs to handle hg and svn too but I'd like to hear more about this since I've landed code which actually broke unit tests in the past during a merge or rebase. It was not fun :)

I got the same problem not for several times. IMOH it would really make sense to run the unit tests before changes are pushed to ensure that they still run successful on the target branch.

My team was bitten twice just today because of that. We expected arc would in fact re run unittests after a merge.

I talked to @btrahan today who mentioned that part of the problem (with lint, at least -- not sure about unit tests) is that linters are allowed to modify code, and you don't want that happening at land time because those changes will be unreviewed.

I've been thinking over this since, and one idea I had was: run the linters, and if they modify any code, then abort the land and display an error. Then also provide a --nolint flag to arc land that lets you skip this step, if you so desire.

Would that work, or is that still too complicated and messy?

The major issue is that this isn't scalable. If your lint+unit tests take 2m to run, the target branch is limited to 30 commits/hour. If you try to commit faster than that, you'll get a clean result and then try to push, but it won't be a fast-forward since someone beat you. And a naive implementation means there's no queueing, so it might take you an arbitrarily long time to actually get your change in, and you have to babysit it the whole time.

This is fine if you have a small team, but not if you're a larger company with a lot of engineers, which can frequently burst way, way above 30 commits/hour for whatever value of 30 actually holds.

The general expectation is that post-review checks are handled by CI, which is a more scalable process. Broadly, why is land-time unit/lint preferable to CI?

What about a configuration option runUnitTestsOnLanding that could be disabled, allTests, changedFiles?
I don't see a benefit for running linters again as the comnination of 2 patches should not change any code style, right?

I'm going to speak for myself here, and not for any of the other subscribers on this thread.

Again, I'm going to talk about linting only. (We don't use arc unit.) For us, arc land currently takes 5-10s and arc lint takes probably another 5-10s on average. Given these current times, adding lint-on-land is unlikely to cause problems. (And in the rare cases when it does cause problems, a --nolint flag should help out.) I understand that it's not scalable, but (assuming it's a configuration option) we could just disable it in the future should we ever reach the point where we start running into problems. Hopefully by then we'll have better CI so we won't need lint-on-land.

Speaking of CI... the way our system is currently set up is that we arc land directly into master, and only then run tests. (None of this "run tests, then merge" fanciness.) Also, our tests take ~30 minutes to run, and (if they fail) the offending commit must be reverted manually by a human rather than automatically by our CI. I realize this is far from ideal, and maybe you're only building Phabricator to support "ideal" workflows, but I'm just trying to tell it how it is.

Let's say someone accidentally introduces a syntax error during the arc land rebase. If we ran linters, we'd end up catching this syntax error in about 5 seconds. The author would notice it, fix it, and then try landing again. All is good in the world!

Instead, because we don't lint-on-land, the author (more often than not) doesn't notice the mistake and lands the broken code. 30-45 minutes later, a human notices the red build and reverts it. Another 30 minutes later, CI finishes building the revert. End result: our build was broken for 1 to 1.5 hours, all because of a simple syntax error that could've been prevented by lint-on-land. Lately, this has been happening to us once or twice a week.

Maybe this isn't a feature everyone wants because most CI systems can handle this use case significantly better, but (at least for now) we would really love it as a configurable option in Phabricator.

For our case, we use both arc lint and arc unit. We have a somewhat "smart" unit test engine that tries to run only affected tests up to a degree so test times are not a huge concern to us. Also the team I work with is ~10 people so there's no way we can go as high as 30 commits/hour, at least in the near future. We have a smooth CI system that is running tests both for each diff revision and every commit on master.

This is still important for us because of the following reasons:

1. It is super confusing when builds fail after landing a "proper" patch and takes time to debug.
2. When builds fail due to this, we cannot deploy (at least version and anything after that).
3. Funnily enough (contributing to #1), the expectation from people is for arc land to run unit tests although there's nowhere saying this just because arc diff does so.
4. arc land already allows authors to modify code before landing if the diff is approved so I think the logical thing to do is run tests before landing. If this behavior is changed, than I may actually change my mind since CI is indeed more scalable. (especially if you run all tests instead of only "relevant" tests).

If we ran linters, we'd end up catching this syntax error in about 5 seconds.

Ah, okay. At Facebook, this was a pre-receive hook on the server using arc git-hook-pre-receive, which basically runs linters in a mode that just looks for serious errors (syntax errors and the like) and rejects commits.

However, I'd like to remove git-hook-pre-receive because it makes linters much harder to write: they have to be able to run in a pre-receive mode with no working copy, and a lot of linters either can't take files over stdin, have different behavior when taking files over stdin, need a configuration file, or have some other kind of "gotcha". It's also hard to test, and hard to debug, even though it works well when it does work. I believe it has zero or near-zero adoption outside of Facebook.

I guess we can also automate the babysitting: if lint passes but the push fails because the remote has moved ahead, we can restart the land. If the commit rate is near the limiting rate it might take a while to get through things, but at least you won't have to babysit it as much.