mozilla / fxa-js-client

DEPRECATED - Migrated to https://github.com/mozilla/fxa
Mozilla Public License 2.0
28 stars 25 forks source link

figure out the eslint rules for the tests directory #163

Closed pdehaan closed 9 years ago

pdehaan commented 9 years ago

Per @vladikoff's https://github.com/mozilla/fxa-js-client/pull/158#issuecomment-116774697 According to @TDA in https://github.com/mozilla/fxa-js-client/pull/158#discussion_r32364977, linting the tests/* directory leads to ~7117 errors/warnings.

vladikoff commented 9 years ago

Thanks Peter

TDA commented 9 years ago

@pdehaan @vladikoff What do you think? Start from scratch and add rules? or start from fxa-config and remove rules?

vladikoff commented 9 years ago

You mentioned 7117 warnings, is there a common pattern for those errors? If so then we need to exclude that rule for the tests/ directory

TDA commented 9 years ago

@vladikoff Let me paste a screenshot. https://www.dropbox.com/s/rzgjb2le1b433cw/Screenshot%202015-06-29%2011.35.25.png?dl=0 Almost 50% of the errors are quotes-related.

pdehaan commented 9 years ago

After debugging this w/ @TDA, it looks like we only have ~130 errors when linting the /tests/*/.js directory. And about 120 of those are semicolons, quotes, formatting issues.

{
  "env": {
    "mocha": true
  },
  "rules": {
    "no-with": 0
  }
}

Listing 1: ./tests/.eslintrc


/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

module.exports = function (grunt) {
  grunt.config('eslint', {
    options: {
      eslintrc: '.eslintrc'
    },
    app: {
      src: [
        'Gruntfile.js',
        'tasks/*.js',
        'config/**/*.js',
        'node/**/*.js'
      ]
    },
    client: {
      options: {
        eslintrc: 'client/.eslintrc'
      },
      src: [
        'client/**/*.js'
      ]
    },
    tests: {
      options: {
        eslintrc: 'tests/.eslintrc'
      },
      src: [
        'tests/**/*.js',
        '!tests/addons/sinon.js'
      ]
    }
  });
};

Listing 2: ./tasks/eslint.js

pdehaan commented 9 years ago

@vladikoff It looks like the majority of those 7000+ errors were coming from the code in ./tests/addons/sinon.js.

After excluding that file, the number drops to a far more manageable 130+/-. And if we temporarily remove the formatting rules (semi, quotes, indent, etc), the number drops to around 9 issues:

$ grunt eslint:tests
Running "eslint:tests" (eslint) task

tests/lib/account.js
  347:35  error  "account" is not defined  no-undef
  353:29  error  "account" is not defined  no-undef

tests/lib/certificateSign.js
  15:10  warning  mail is defined but never used        no-unused-vars
  18:10  warning  ErrorMocks is defined but never used  no-unused-vars

tests/lib/errors.js
  15:10  warning  mail is defined but never used          no-unused-vars
  17:10  warning  RequestMocks is defined but never used  no-unused-vars

tests/lib/recoveryEmail.js
  18:10  warning  ErrorMocks is defined but never used  no-unused-vars

tests/lib/verifyCode.js
  13:10  warning  accountHelper is defined but never used  no-unused-vars
  18:10  warning  ErrorMocks is defined but never used     no-unused-vars

✖ 9 problems (2 errors, 7 warnings)

Warning: Task "eslint:tests" failed. Use --force to continue.

Aborted due to warnings.
TDA commented 9 years ago

Ok this one looks super-legit, account really isnt there tests/lib/account.js 347:35 error "account" is not defined no-undef 353:29 error "account" is not defined no-undef @pdehaan @vladikoff

vladikoff commented 9 years ago

Yeah we should check the account file. and yes exclude sinon.js

pdehaan commented 9 years ago

@TDA: Also, as part of this, we'll need to cleanup the tasks/jscs.js file to make sure it also lints the tests/\ directory:

Something like this:

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

module.exports = function (grunt) {
  'use strict';

  grunt.config('jscs', {
    options: {
      config: '.jscsrc'
    },
    src: [
      '{,client/**/, node/,tasks/,tests/**/}*.js'
    ]
  });
};

You'll probably get something like 801 errors, until we tweak those quote marks and stuff. Then maybe we start looking into which rules are in both .eslintrc and .jscsrc and remove them from JSCS, since we don't need two linters tracking quotes, keywords, indentation, etc.

>> 801 code style errors found!