This was startling to me at first, but on further reflection, I'd like
to caution against reacting for the wrong reasons.
One driver for our current workflows is that the patch attached to
bugzilla is rarely what actually landed. With things that go through
mozreview, that need no longer be the case. If you need to update a
patch with review comments, update it and re-push to mozreview.
Question: how does mozreview handle the case where a commit has r+ and
then a newer version is pushed? Does it inherit the r+ or not? IMHO,
neither answer is correct all of the time, and it's problematic that I
even have to wonder about what happens. This should be surfaced clearly
in the UX... somehow. (Do we need a new |hg push --update| flag that
means "inherit previous reviews"? Or should you just push and then go to
the web interface and click to inherit the reviews?)
Anyway, my thinking about reviewers landing:
Landing on r+ is freaky because it is very common for the reviewer to
request (or just suggest as a possibility, without requiring) changes
before landing. That's not a problem because the reviewer would not land
it in that case.
Landing on r+ is freaky because sometimes review is requested before a
patch is known to work or not. That's not as bad as it sounds, because
autoland won't land until tests pass. Question: if the implementation
and its tests are in separate commits, is there a chance that autoland
will land just the implementation but not the failing tests? The best
practice around this should be documented.
So if tests properly cover the change, then the only cost for
prematurely queuing up a commit for landing is machine time for doomed
test runs. In other words, it is more important than it used to be to
have tests for your changes. (But still not critical, since you can
leave a "please don't land yet" note.)
But that's for testable stuff. The other meaning of "known to work" is
whether the change achieves its intended purpose, and that's not always
discoverable from an automated test run. This is quite common in my
experience. Heck, performance optimizations often fall within this
category, especially if it's a somewhat speculative change that
complicates things in exchange for hoped-for but uncertain speed gains.
Still, "please don't land" seems sufficient for these, if it's even
necessary given the nature of the patch.
I would also suggest that reviewers should feel freer to land on r+ if
there are adequate tests accompanying the change. And of course, there
always "should" be adequate test in the first place, but that's often
impractical, so I'm just saying to use tests as a signal when deciding
whether to land on review.
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
