> On Sep 15, 2020, at 8:48 AM, Jonathan Kew <jfkth...@gmail.com> wrote: > > On 15/09/2020 16:03, Brian Grinstead wrote: > > > We’re rolling out a change to the review process to put more focus on > > automated testing. [...] > > As others have said, it's great that we're setting a clearer policy here - > thanks! > > One thing did strike me as a little surprising, and could perhaps be > clarified: > > > * testing-exception-other: Commits where none of the defined exceptions > > above apply but it should still be landed. This should be scrutinized by > > the reviewer before using it - consider whether an exception is actually > > required or if a test could be reasonably added before using it. This > > requires a comment from the reviewer explaining why it’s appropriate to > > land without tests. > > The point that caught my eye here was that it explicitly requires the > explanatory comment to be *from the reviewer*. I would have thought that in > many cases it would make sense for the *patch author* to provide such an > explanatory comment (which the reviewer would of course be expected to > scrutinize before granting r+). Is that scenario going to be considered > unacceptable? >
I think that’s OK and something we can clarify in the text if needed. It’s helpful for authors to explain the reasoning if they think an exception should be applied (essentially, making a request for a particular exception). We'd need to decide if either (a) the reviewer grants the request by adding the exception with a link pointing to the authors comment, (b) the reviewer grants the request by adding the exception without a comment, or (c) the author adds the exception at the same time they make the request, and the reviewer implictly grants the request by not removing it. As the policy is written, (a) is already fine. If based on usage feedback this is inconvenient and not providing value I’m open to adjusting it accordingly. Though did want to note that I’d be more interested in optimizing the workflow for `testing-exception-elsewhere` which also requires a comment (as it is expected / commonly used, like in Stacks) than `testing-exception-other` which should be used less often and require more scrutiny. Brian > Thanks, > > JK > > _______________________________________________ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
