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 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. - 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
