Brian, Now that we've been doing this for a while, do we have any aggregated data on the testing tags? i.e. how often are we adding tests vs not.
-Jeff On Tue, Sep 15, 2020 at 11: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. > > We’ve been piloting the policy for a few weeks with positive feedback from > reviewers. Still, there may be some rough edges as we roll this out more > widely. I’d like to hear about those, so please contact me directly or in the > #testing-policy channel in Slack / Matrix if you have feedback or questions > about how the policy should apply to a particular review. > > We’re working on ways to make this more convenient in the UI and to prevent > landing code without a tag in Lando. In the meantime if you'd like a reminder > to add the tag while reviewing code, Nicolas Chevobbe has built a > WebExtension that automatically opens up the Project Tags UI when appropriate > at https://github.com/nchevobbe/phab-test-policy. > > ## Why? > > Automated tests in Firefox and Gecko reduce risk and allow us to quickly and > confidently improve our products. > > While there’s a general understanding that changes should have tests, this > hasn’t been tracked or enforced consistently. We think defining an explicit > policy for including automated tests with code changes will help to achieve > those goals. > > Also, we’ll be able to better understand exactly why and when tests aren’t > being added. This can help to highlight components that need new testing > capabilities or technical refactoring, and features that require increased > manual testing. > > There are of course trade-offs to enforcing a testing policy. Tests take time > to write, maintain, and run which can slow down day-to-day development. And > given the complexity of a modern web browser, some components are impractical > to test today (for example, components that interact with external hardware > and software). > > The policy below hopes to mitigate those by using a lightweight enforcement > mechanism and the ability to exempt changes at the discretion of the code > reviewer. > > ## Policy (This text is also located at > https://firefox-source-docs.mozilla.org/testing/testing-policy/) > > Everything that lands in mozilla-central includes automated tests by default. > Every commit has tests that cover every major piece of functionality and > expected input conditions. > > One of the following Project Tags must be applied in Phabricator before > landing, at the discretion of the reviewer: > > * `testing-approved` if it has sufficient automated test coverage. > * One of `testing-exception-*` if not. After speaking with many teams across > the project we’ve identified the most common exceptions, which are detailed > below. > > ### Exceptions > > * testing-exception-unchanged: Commits that don’t change behavior for end > users. For example: > * Refactors, mechanical changes, and deleting dead code as long as they > aren’t meaningfully changing or removing any existing tests. Authors should > consider checking for and adding missing test coverage in a separate commit > before a refactor. > * Code that doesn’t ship to users (for example: documentation, build > scripts and manifest files, mach commands). Effort should be made to test > these when regressions are likely to cause bustage or confusion for > developers, but it’s left to the discretion of the reviewer. > * testing-exception-ui: Commits that change UI styling, images, or localized > strings. While we have end-to-end automated tests that ensure the frontend > isn’t totally broken, and screenshot-based tracking of changes over time, we > currently rely only on manual testing and bug reports to surface style > regressions. > * testing-exception-elsewhere: Commits where tests exist but are somewhere > else. This requires a comment from the reviewer explaining where the tests > are. For example: > * In another commit in the Stack. > * In a followup bug. > * In an external repository for third party code. > * When following the [Security Bug Approval > Process](https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html) > tests are usually landed later, but should be written and reviewed at the > same time as the commit. > * 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. Some examples that have been identified include: > * Interacting with external hardware or software and our code is missing > abstractions to mock the interaction out. > * Inability to reproduce a reported problem, so landing something to test a > fix in Nightly. > > Thanks, > Brian > > _______________________________________________ > 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