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

Reply via email to