> On Sep 15, 2020, at 11:44 AM, Andrew Halberstadt <a...@mozilla.com> wrote: > > On Tue, Sep 15, 2020 at 12:28 PM Andrew McCreight <amccrei...@mozilla.com> > wrote: >> I don't know that tests being an official requirement relieves the burden > of guilt of asking for tests, as everybody already knows that tests are > good and that you should always write tests > > I think this is largely true at Mozilla, but we all have our lapses from > time to time. Thankfully since we all know this, a tiny nudge is all that > we need. > >> Separately, one category of fixes I often deal with is fixing leaks or > crashes in areas of the code I am unfamiliar with. I can often figure out > the localized condition that causes the problem and correct that, but I > don't really know anything about, say, service workers or networking to > write a test. Do I need to hold off on landing until I can find somebody > who is willing and able to write a test for me, or until I'm willing to > invest the effort to become knowledgeable enough in an area of code I'm > unlikely to ever look at again? Neither of those feel great to me. > > This sounds like an argument in favour of putting the burden of exceptions > on the reviewer. Authors can submit without a test (ideally calling out > it's because they don't know how) and then the reviewer can either: > A) Explain how to add tests for said patch, or > B) Knowing that writing a test would be difficult, grant them an exception
Or (C) the reviewer could write the test themselves and land it in a separate commit alongside the patch, marking the original patch as `testing-exception-elsewhere`. We definitely don't want to discourage writing drive-by / “good samaritan” patches where you are fixing a bug in an area outside of your normal work. This came up a few times in discussions and is one of the reasons I prefer reviewer-specified instead of author-specified exceptions. > If a reviewer finds themselves explaining / granting exceptions often, it > could be a signal that the test infrastructure in that area needs > improvement. > Btw I think your points are all valid, I guess we'll have to see how it > turns out in practice. Hopefully we can adjust the process as needed based > on what we learn from it. For sure. I’m happy to discuss any specific pain points you run into with the policy. > - Andrew > > On Tue, Sep 15, 2020 at 12:28 PM Andrew McCreight <amccrei...@mozilla.com> > wrote: > >> On Tue, Sep 15, 2020 at 8:03 AM Brian Grinstead <bgrinst...@mozilla.com> >> wrote: >> >>> (This is a crosspost from firefox-dev) >>> >>> Hi all, >>> >>> We’re rolling out a change to the review process to put more focus on >>> automated testing. This will affect you if you review code that lands in >>> mozilla-central. >>> >>> ## TLDR >>> >>> Reviewers will now need to add a testing Project Tag in Phabricator when >>> Accepting a revision. This can be done with the “Add Action” → “Change >>> Project Tags” UI in Phabricator. There's a screenshot of this UI at >>> https://groups.google.com/g/firefox-dev/c/IlHNKOKknwU/m/zl6cOlT2AgAJ. >>> >> >> I of course think having more tests is a good thing, but I don't like how >> this specific process places all of the burden of understanding and >> documenting some taxonomy of exceptions on the reviewer. Reviewing is >> already a fairly thankless and time consuming task. It seems like the way >> this is set up is due to the specifics of our how reviewing process is >> implemented, so maybe there's no way around it. >> >> Also, contrary to what ahal said, I don't know that tests being an official >> requirement relieves the burden of guilt of asking for tests, as everybody >> already knows that tests are good and that you should always write tests. >> The situation is different from the linters, as the linters will fix almost >> all issues automatically, so asking somebody to fix linter issues isn't >> much of a burden at all. I guess it is impossible to make testing better >> without somebody directly pressuring somebody, though. >> >> My perspective might be a little distorted as I think a lot of the patches >> I write would fall under the exceptions, either because they are >> refactoring, or I am fixing a crash or security issue based on just a stack >> trace. >> >> Separately, one category of fixes I often deal with is fixing leaks or >> crashes in areas of the code I am unfamiliar with. I can often figure out >> the localized condition that causes the problem and correct that, but I >> don't really know anything about, say, service workers or networking to >> write a test. Do I need to hold off on landing until I can find somebody >> who is willing and able to write a test for me, or until I'm willing to >> invest the effort to become knowledgeable enough in an area of code I'm >> unlikely to ever look at again? Neither of those feel great to me. >> >> Another testing difficulty I've hit a few times this year (bug 1659013 and >> bug 1607703) are crashes that happen when starting Firefox with unusual >> command line or environment options. I think we only have one testing >> framework that can test those kinds of conditions, and it seemed like it >> was in the process of being deprecated. I had trouble finding somebody who >> understood it, and I didn't want to spend more than a few hours trying to >> figure it out for myself, so I landed it without testing. To be clear, I >> could reproduce at least one of those crashes locally, but I wasn't sure >> how to write a test to create those conditions. Under this new policy, how >> do we handle fixing issues where there is not always a good testing >> framework already? Writing a test is hopefully not too much of a burden, >> but obviously having to develop and maintain a new testing framework could >> be a good deal of work. >> >> Anyways those are some disjointed thoughts based on my experience >> developing Firefox that probably don't have good answers. >> >> Andrew >> _______________________________________________ >> 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 _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
