Hi Jeff, Thanks to help from Marco Castelluccio we now have an artifact in taskcluster that has metadata about all commits that land in m-c, which I've used to gather this data. We're still working on tooling to better understand patterns with different types of bugs & patches, but I'm happy to share what we already have.
Here's a spreadsheet showing weekly data since September: https://docs.google.com/spreadsheets/d/1xpm7V-zHCB3nyENdoVkG2ViT5iyg940SIpobY1fzR2g/edit. A few notes: * "none" is when a patch is reviewed/landed without a tag being applied. I've been hoping this would naturally get down to 0 as people get used to the policy and from the notifications we added in phabrictor. It dropped quickly to ~10% but is hovering right now. I've spoken with the Lando team about changing our warning checkbox into a hard blocker to enforce this. * "unknown" counts revisions that don't have an accessible phabricator revision. This is mostly automated commits like wpt-sync (for example https://hg.mozilla.org/mozilla-central/rev/19b1604e8c8e) but also includes revisions with restricted permissions that aren't able to be loaded during generation time. For the sake of reporting I think these can be pretty much ignored. * From most common to least common we see: “unchanged”, “approved”, “other”, “elsewhere”, “ui”. This has remained pretty stable across weeks. I’m not surprised that “unchanged” is common since it’s a bit overloaded - covering both (a) code that don’t ship to users and (b) code that’s being modified but keeping the same functionality. Finally, I’m happy to hear feedback about how people think this has been going from a reviewer/author standpoint. Most of the feedback has been around ambiguity with “unchanged”. For example when “unchanged” versus “approved” should be applied when touching only tests: https://bugzilla.mozilla.org/show_bug.cgi?id=1672439. Or if we should somehow split up “unchanged” to differentiate between code that already has tests and code that doesn’t. #testing-policy is a good channel for that discussion if you are interested in the topic. Thanks, Brian > On Nov 12, 2020, at 8:21 AM, Jeff Muizelaar <jmuizel...@mozilla.com> wrote: > > 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
