Configure amo eslint config #1451

muffinresearch · 2017-08-08T17:00:18Z

Fixes #1450
Fixes #1391 (it made testing easier)

It found a couple of real issues:

There was a broken regex which would match browser/components/extensions/schemas/lkfjghdsfhskdjhfjson and not just browser/components/extensions/schemas/lkfjghdsfhskdjhf.json
There's a test that had a bad loop I've commented out the data that doesn't validate - it's a schema test and I will call it out in the diff where possible. This one will need a bug filed to fix up in due course.

A lot of the noise is spaces around { etc.
The bigger changes were changes to a lot of the loops to move away from for..in and for..of.

There's a lot of changes here so it'll be worth trying to be diligent with review incase something got broken in the process.

muffinresearch · 2017-08-08T17:02:28Z

tests/schema/test.permissions.js

-    'https://foo.com',
-    'ftp://do.people.still.use.this',
-    'app://wat',
+    // FIXME: the loop in this test was previously broken.


This is the bad loop that needs a bug filing to fix. The matchPattern seen in the test was always the last value in the list.

coveralls · 2017-08-08T17:03:30Z

Coverage remained the same at 100.0% when pulling 9785d4a on muffinresearch:configure-amo-eslint-config into e50c28c on mozilla:master.

EnTeQuAk

r+wc

EnTeQuAk · 2017-08-10T10:55:36Z

package.json

-    "test": "jest tests/ --runInBand --coverage",
-    "test-no-coverage": "jest tests/ --runInBand",
-    "test-ci": "npm run test && cat ./coverage/lcov.info | coveralls",
+    "test": "jest tests/ --runInBand --watch",


I'm honestly not a fan of a default watch, can we maybe not do that? At least on my machine it's a constant memory leak

Would be good to get to the bottom of why that is. Not using watch defeats one of the best features of Jest.

I suppose one of the other goals I was aiming for was to try and make the test command setup more consistent between addons-frontend and addons-linter since that's another difference we can eliminate.

If I added the test-once command would that provide what you need? See https://github.com/mozilla/addons-frontend/#development-commands for more details.

I've added yarn test-once and test-coverage-once.

Perfect, thanks a lot! I think that works for me.

EnTeQuAk · 2017-08-10T13:48:56Z

src/io/xpi.js


  checkPath(path) {
-    if (!this.files.hasOwnProperty(path)) {
+    if (!Object.prototype.hasOwnProperty.call(this.files, path)) {


what's the difference here?

See http://eslint.org/docs/rules/no-prototype-builtins

EnTeQuAk · 2017-08-10T13:53:39Z

src/scanners/binary.js

-    for (let v in values) {
+    const valuesKeys = Object.keys(values);
+    for (let i = 0; i < valuesKeys.length; i++) {
+      const v = valuesKeys[i];


why no forEach here?

Because there's no way to break out of a forEach without throwing.

Thinking about it this is probably something where we could be using https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some

It could probably be re-written as:

if (Object.keys(values).some((v) => values[v] !== buffer[v])) { return; }

I'm going to switch this to use Array.prototype.some. It's purpose built for this use-case.

EnTeQuAk · 2017-08-10T13:55:03Z

src/schema/firefox-schemas-import.js

-  new RegExp('browser/components/extensions/schemas/.*\.json'),
-  new RegExp('toolkit/components/extensions/schemas/.*\.json'),
+  new RegExp('browser/components/extensions/schemas/.*\\.json'),
+  new RegExp('toolkit/components/extensions/schemas/.*\\.json'),


great catch!

EnTeQuAk · 2017-08-10T13:55:48Z

src/schema/formats.js

-  for (var part of parts) {
+
+  for (let i = 0; i < parts.length; i++) {
+    let part = parts[i];


also no forEach here?

Same reason as before.

EnTeQuAk · 2017-08-10T13:58:34Z

tests/io/test.xpi.js


-describe('Xpi.getFilesByExt()', function() {
-
+describe('Xpi.getFilesByExt()', function getFilesByExtCallback() {


Why are they named now? Can't we just use lambdas here as well, afair we do that quite often in other tests?

One of the lint rules prevents unnamed functions. In our tests we have a few places where we rely on this, and the semantics of a fat arrow breaks that code from working . We should probably look at refactoring those tests and set consts instead of relying on this.

FWIW, you could just create a nested ./tests/.eslintrc which disables that one unnamed function rule, and all the other rules from the root .eslintrc file would still be inherited.

@pdehaan If we end up concluding we don't need the rule I'd rather disable it centrally in the shared config since we're trying to make our config more consistent across repos. Of course there might be some cases where we do need overrides but we'd probably want to keep that to an absolute minimum.

coveralls · 2017-08-10T18:11:46Z

Coverage remained the same at 100.0% when pulling ba4dcbc on muffinresearch:configure-amo-eslint-config into e50c28c on mozilla:master.

coveralls · 2017-08-10T18:25:45Z

Coverage remained the same at 100.0% when pulling 1a809b5 on muffinresearch:configure-amo-eslint-config into e50c28c on mozilla:master.

muffinresearch · 2017-08-10T18:33:26Z

@EnTeQuAk if you're happy with this and want to land it feel free to go ahead and squash/merge it. Otherwise we can fix up any additional bits next week.

coveralls · 2017-08-11T05:03:58Z

Coverage remained the same at 100.0% when pulling 3056883 on muffinresearch:configure-amo-eslint-config into 80e68d9 on mozilla:master.

coveralls · 2017-08-11T11:02:54Z

Coverage remained the same at 100.0% when pulling 6c379cf on muffinresearch:configure-amo-eslint-config into 91a89f5 on mozilla:master.

EnTeQuAk · 2017-08-11T14:49:54Z

Thank you so much for the explanations and work that went into this! This is looking great :)

So I just merged this, if there's anything else that needs to be done I'm sure we can easily do it in a separate PR.

muffinresearch added 2 commits August 8, 2017 17:52

Sync lockfile

b76a533

Add and setup amo eslint config

9785d4a

muffinresearch requested a review from EnTeQuAk August 8, 2017 17:00

muffinresearch commented Aug 8, 2017

View reviewed changes

muffinresearch modified the milestone: 2017.08.17 Aug 8, 2017

This was referenced Aug 8, 2017

Import Firefox 56 schemas #1449

Merged

Update eslint to the latest version 🚀 #1444

Closed

EnTeQuAk approved these changes Aug 10, 2017

View reviewed changes

Use Array.prototype.some instead of loop

ba4dcbc

muffinresearch added 2 commits August 10, 2017 19:11

Make test command compare with addons-frontend

a2c8143

Update README to refer to yarn over npm

1a809b5

Merge branch 'master' into configure-amo-eslint-config

3056883

Merge branch 'master' into configure-amo-eslint-config

6c379cf

EnTeQuAk merged commit 6109428 into mozilla:master Aug 11, 2017


		describe('Xpi.getFilesByExt()', function() {

		describe('Xpi.getFilesByExt()', function getFilesByExtCallback() {

Configure amo eslint config #1451

Configure amo eslint config #1451

Uh oh!

Conversation

muffinresearch commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 8, 2017

Uh oh!

EnTeQuAk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muffinresearch Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 10, 2017

Uh oh!

coveralls commented Aug 10, 2017

Uh oh!

muffinresearch commented Aug 10, 2017

Uh oh!

coveralls commented Aug 11, 2017

Uh oh!

coveralls commented Aug 11, 2017

Uh oh!

EnTeQuAk commented Aug 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

muffinresearch Aug 10, 2017 •

edited

Loading