A reminder about code reviews and tests

[Patrick Brosset]

As more and more of our code is moving to GitHub these days, a lot of our
tooling and workflows are changing, hopefully for the better.
But in this rapidly changing environment, it is very important that we
preserve our *quality standards*.

To be very clear, quality is key, and if you ever find yourself wondering
whether you should ship a feature now, or spend more time making quality
better, then go for quality!
Features and bug fixes can wait, whereas technical debt will follow us
around forever and be a source of frustration for everyone working on the
code.

So, two of the quality standards we need to keep during the transition are:
code reviews and automated testing.

*1. Code reviews*

Code reviews today happen on bugzilla/mozreview and are a mandatory step
for landing any patch.
*We need to do them on GitHub too for all changes.*

- If you are creating a new repo, please configure it so code reviews are
mandatory.
- If we have repos not configured this way yet, please let me know.
- If you are an admin on a repo and therefore have the possibility to self
approve commits, please don't and ask reviews from your peers instead.

For info, Mozilla has a module system
<https://wiki.mozilla.org/Modules/All#DevTools> that defines who owns
"modules" and who can do reviews on them (peers). We have enough peers
around the world that it should never be difficult to find someone to
review a code change (even in the same time zone if time is an issue).

Of course, extracting DevTools outside of the Firefox code base means that
the module system won't apply in the future, but until we have something of
our own for this, let's use it still.

I don't need to say this, but code reviews are essential not just for
quality reasons and for preventing bugs, but also for communication and
understanding among the folks working on DevTools.



*2. Automated testings*
This is true no matter if the code is in m-c or GitHub: new code must be
tested properly. Either with integration tests or unit tests or both,
depending on what applies best.


*- Bug fixes should come with non-regression tests that cover the failing
cases.*
*- New features should come with various tests that check the main use
cases as well as edge cases.*

We have many ways to write tests, depending on where the code is and its
nature, and it can sometimes be hard to find the right type of test. Please
make sure you ask colleagues, reviewers, people on IRC or Slack about that.
We also have some docs about this
<http://searchfox.org/mozilla-central/source/devtools/docs/tests>.

Moving to GitHub might give us an opportunity to have better test coverage
in fact, with tools like codecov.io which easily interface with repos on
GitHub.
Today, we have many tests in m-c, but no way of knowing how much of our
code is covered by them.

Thank you all!
Patrick

[Mike Taylor]

Hi Patrick,

On 5/12/17 2:52 AM, Patrick Brosset wrote:
> Of course, extracting DevTools outside of the Firefox code base means that
> the module system won't apply in the future, but until we have something of
> our own for this, let's use it still.

FWIW, moving code outside of mozilla-central doesn't mean the module
system no longer applies, so long as it is still considered to be part
of the Mozilla project.

https://wiki.mozilla.org/Modules

--
Mike Taylor
Web Compat, Mozilla

[Patrick Brosset]

Thanks Mike for the correction. I was wrong to assume that the module
system only applied to the code in m-c.