Skip to content

cache lint results for faster repeat execution#10773

Merged
brad-decker merged 1 commit intodevelopfrom
cache-lint
Mar 31, 2021
Merged

cache lint results for faster repeat execution#10773
brad-decker merged 1 commit intodevelopfrom
cache-lint

Conversation

@brad-decker
Copy link
Copy Markdown
Contributor

@brad-decker brad-decker commented Mar 30, 2021

Increase performance of lint on successive runs

First run, normal:

➜ TIMING=1 yarn lint
yarn run v1.22.4
$ prettier --check ./**/*.json && eslint . --ext js --cache && yarn lint:styles
Checking formatting...
All matched files use Prettier code style!
Rule                           | Time (ms) | Relative
:------------------------------|----------:|--------:
prettier/prettier              | 31488.596 |    24.0%
import/namespace               | 11785.826 |     9.0%
mocha/no-nested-tests          | 10261.004 |     7.8%
mocha/no-identical-title       |  8864.979 |     6.8%
mocha/no-skipped-tests         |  6542.303 |     5.0%
mocha/no-exclusive-tests       |  6495.606 |     4.9%
mocha/no-hooks-for-single-case |  5976.042 |     4.6%
mocha/max-top-level-suites     |  3757.385 |     2.9%
mocha/no-top-level-hooks       |  3475.382 |     2.6%
mocha/no-async-describe        |  3465.187 |     2.6%
$ stylelint '*/**/*.scss'
✨  Done in 164.33s.

Second run, cached:

➜ TIMING=1 yarn lint
yarn run v1.22.4
$ prettier --check ./**/*.json && eslint . --ext js --cache && yarn lint:styles
Checking formatting...
All matched files use Prettier code style!
Rule | Time (ms) | Relative
:----|----------:|--------:
$ stylelint '*/**/*.scss'
✨  Done in 7.97s.

Third run, after changing one file to have a lint issue, only one file checked:

$ prettier --check ./**/*.json && eslint . --ext js --cache && yarn lint:styles
Checking formatting...
All matched files use Prettier code style!

/Users/braddecker/Dev/consensys/metamask-extension/ui/lib/account-link.js
  9:10  error  'createAccountLinkForChain' is not defined  no-undef

✖ 1 problem (1 error, 0 warnings)

Rule                           | Time (ms) | Relative
:------------------------------|----------:|--------:
prettier/prettier              |   135.526 |    74.8%
mocha/no-identical-title       |     2.563 |     1.4%
mocha/max-top-level-suites     |     2.526 |     1.4%
mocha/no-nested-tests          |     2.157 |     1.2%
react/no-unused-prop-types     |     1.784 |     1.0%
mocha/no-hooks-for-single-case |     1.640 |     0.9%
mocha/no-exclusive-tests       |     1.452 |     0.8%
mocha/no-mocha-arrows          |     1.448 |     0.8%
no-control-regex               |     1.291 |     0.7%
mocha/no-skipped-tests         |     1.239 |     0.7%
error Command failed with exit code 1.

The problem with this setup is with the import plugin. For example, if you change a file such that it no longer exports a thing, but the file is still valid...running lint won't find the errors in files that did not change but that rely on that import. eg:

#File a
export const foo = 'bar';

#File b
import { foo } from 'file-a';

changing foo to baz in File a will not trigger a lint failure with the cache option, but when removing the cache or running in CI it will find it. I'm almost still in favor of this! I'd rather have 150 seconds of my day back frequently and be like "doh" when it fails in CI. 😅

@brad-decker brad-decker marked this pull request as ready for review March 30, 2021 17:17
@brad-decker brad-decker requested a review from a team as a code owner March 30, 2021 17:17
@brad-decker brad-decker requested a review from Gudahtt March 30, 2021 17:17
@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Mar 31, 2021

I'm almost still in favor of this! I'd rather have 150 seconds of my day back frequently and be like "doh" when it fails in CI. sweat_smile

Agreed! Seems like a reasonable tradeoff to me.

I'm curious why this cache is never hit on CI though, if the flag is still used. Where is the cache? 🤔

@brad-decker
Copy link
Copy Markdown
Contributor Author

brad-decker commented Mar 31, 2021

@Gudahtt the cache is in a file that gets generated .eslintcache -- i don't think it should be persisted between runs unless we tell it to? We can tell eslint where to put the cache file and what to name it or if I can change CI so that it definitely doesn't persist that file I can.

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Mar 31, 2021

Gotcha. That sounds OK. I was afraid it was being cached in node_modules somewhere, which CI does cache. But I'm not concerned if it's a dotfile that is in our .gitignore.

Gudahtt
Gudahtt approved these changes Mar 31, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@brad-decker brad-decker merged commit 95fbf92 into develop Mar 31, 2021
@brad-decker brad-decker deleted the cache-lint branch March 31, 2021 15:19
@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@brad-decker @Gudahtt