> On Sep 16, 2020, at 7:12 AM, surkov.a...@gmail.com > <surkov.alexan...@gmail.com> wrote: > > Better test coverage is the best. Agreed, having a policy to force developers > to add tests must be an efficient way to improve test coverage. I worried > though it may reduce amount of contributions from the community since it > complicates the development process: sometimes you have to spend more time to > create a test than to fix a bug.
I think this is similar to the issue raised by mccr8 and discussed upthread. People who aren't familiar with a particular area of the codebase should still feel free to write a patch without a test to fix a bug. It's then up to the reviewer to either provide instructions to write a test, write a test themselves, or grant an exception. > Also it could make developers (intentionally or unintentionally) to pick up > tasks that don't need tests. The worse thing is perhaps you cannot > effectively measure these side affects of the change. The exceptions are meant to at least partially address these points (see also ekr's point (2) above). Brian > Thanks, > Alexander. > > On Tuesday, September 15, 2020 at 11:03:45 AM UTC-4, Brian Grinstead 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
