New: do not remove non visited files from cache. (Fixes #6780) #6921

royriojas · 2016-08-16T22:51:41Z

What issue does this pull request address?
It allows cache file to keep non used files in the cache between executions of eslint, also files that don't exist anymore are removed from the cache both when loaded and when saved.

What changes did you make? (Give an overview)

updated file-entry-cache to 2.0.0
added unit tests to cover the expected functionality

Is there anything you'd like reviewers to focus on?

correctness of the tests
Also would be nice to review also this jaredwray/file-entry-cache@408374d where the actual removal of non found files happen.

cc. @IanVS

eslintbot · 2016-08-16T22:51:43Z

Thanks for the pull request, @royriojas! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.
Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

jquerybot · 2016-08-16T22:51:46Z

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

eslintbot · 2016-08-16T22:57:53Z

LGTM

IanVS · 2016-08-17T02:16:56Z

Thanks @royriojas! I'm going to take a look at this and try it out tomorrow. We're also still waiting for a reply from the rest of the team on the issue, to determine whether this should wait for a major version bump or not.

royriojas · 2016-08-17T07:29:29Z

Sure man! hope this solves the issue

vitorbal · 2016-08-17T08:06:05Z

Adding "do not merge" for now as the issue is still not accepted!

IanVS · 2016-08-18T04:15:31Z

I did some unscientific timings while linting the node.js repo (the largest project I could think of that uses ESLint). As far as I can tell, there are 3,818 JavaScript files in that repo.

Linting them without a cache takes 11.5 - 12.0 sec on my machine (a 2014 Macbook Pro with SSD and 16GB RAM). I found that the new removeNotFoundFiles function in file-entry-cache added about 0.13 ms in this case.

When using --cache, total linting time is reduced to 1.3 sec. removeNotFoundFiles runs twice (once at the start to prune out missing files, once at the end when saving off the cache file), for a combined time of 12.5 ms.

Next, I deleted about half the files in the repo, and ran the lint again (still using --cache). In this case, the total linting time was 0.8 sec and the time spent in removeNotFoundFiles was 17.5 ms the first time, and 2.5 ms in subsequent runs.

Incidentally, the PR seems to do what it is intended to do. Deleted files are removed from the cache, but running eslint against a subset of cached files does not clear out the other cached files (the original point of #6780).

All-in-all, I really hope we can move forward with this, because I think the performance hit from checking for removed files is minuscule even in this contrived case, and the gains from consistent caching are huge. Thanks for the great work, @royriojas. Think of all the hours of human lives you've saved already!

IanVS · 2016-08-18T14:15:57Z

I've accepted the issue, so I believe this is ready for review/merge.

platinumazure · 2016-08-18T14:21:54Z

My only concern with this PR is that the newly added tests have multiple action/assertion sections (in other words, these are not unit tests, they're functional tests). If that's acceptable to everyone else, I'm okay with it.

IanVS · 2016-08-18T14:45:44Z

I agree, and was going to make that same point. @royriojas, do you think you could break up the tests a bit to only test one thing at a time?

royriojas · 2016-08-18T15:51:44Z

@IanVS, @platinumazure I just copied the tests that were there before and modified them a bit :), so I was thinking it was ok to have that many assertions.

I can break the tests, will do that tonight.

eslintbot · 2016-08-22T01:55:42Z

LGTM

royriojas · 2016-08-22T01:56:50Z

@IanVS, @platinumazure I removed some of the assertions and only left the assertion to check what the test was actually expecting. Please let me know if that is good enough

IanVS · 2016-08-23T16:56:52Z

LGTM

platinumazure · 2016-08-23T16:59:22Z

I'm not seeing the tests having been broken down-- am I missing something?

IanVS · 2016-08-23T17:04:09Z

He squashed the changes. There used to be more unrelated assertions. There are indeed still a few assertions, but I think they're reasonable given the test. For example in the first test, he checks that the file is in the cache after the first lint run in order to establish that the removal is happening properly in the next linting run. Caching is inherently a two-part process, and in my mind at least these tests are reasonable. Happy to hear opinions from others though.

platinumazure · 2016-08-23T17:05:12Z

Fine by me.

Usually in cases like this, I like to see two tests: One that checks the file is still present after 1 run, and one test that does 2 runs and checks the file is gone after the second run. But I admit it might be overkill for this case. So the changes LGTM.

IanVS · 2016-08-23T17:06:29Z

Usually in cases like this, I like to see two tests: One that checks the file is still present after 1 run, and one test that does 2 runs and checks the file is gone after the second run.

Yes, but then isn't there the danger that one of the tests and code is changed at some point, and then they no longer are connected? I guess the only real way to assure that doesn't happen is to make a helper function that they both share, but it seems like overkill to me.

vitorbal · 2016-08-23T17:14:52Z

tests/lib/cli-engine.js

+             * helper method to delete the cache files created during testing
+             * @returns {void}
+             */
+            function delCache() {


Maybe a nitpick, but could we call this deleteCache instead? There's no need to abbreviate delete to del here.

vitorbal · 2016-08-23T17:17:37Z

tests/lib/cli-engine.js

-             * helper method to delete the cache files created during testing
+             * helper method to delete a file without caring about exceptions
+             *
+             * @param      {string}  filePath  The file path


Could we get rid of the extra whitespaces between @param and {string} here?

eslintbot · 2016-08-23T18:58:01Z

LGTM

royriojas · 2016-08-23T18:58:27Z

@vitorbal Code was updated!

ilyavolodin · 2016-08-23T21:15:14Z

LGTM, waiting for @vitorbal to approve.

vitorbal · 2016-08-24T06:46:37Z

Lgtm!
On Tue, Aug 23, 2016 at 11:15 PM Ilya Volodin notifications@github.com
wrote:

LGTM, waiting for @vitorbal https://github.com/vitorbal to approve.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6921 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAmNdh4DYCqVG_C6AiJY7YeVQQMb4u_gks5qi2LkgaJpZM4Jl7fc
.

vitorbal · 2016-08-24T13:53:53Z

Thanks for your contribution, @royriojas! 🎉

jquerybot added the CLA: Error label Aug 16, 2016

royriojas changed the title ~~FEAT: do not remove non visited files from cache. Closes #6780~~ New: do not remove non visited files from cache. Closes #6780 Aug 16, 2016

royriojas changed the title ~~New: do not remove non visited files from cache. Closes #6780~~ New: do not remove non visited files from cache. Fixes #6780 Aug 16, 2016

royriojas force-pushed the issue6780 branch from e676cd6 to c4caf4d Compare August 16, 2016 22:57

royriojas changed the title ~~New: do not remove non visited files from cache. Fixes #6780~~ New: do not remove non visited files from cache. (Fixes #6780) Aug 16, 2016

jquerybot added CLA: Valid and removed CLA: Error labels Aug 17, 2016

vitorbal added the do not merge This pull request should not be merged yet label Aug 17, 2016

IanVS mentioned this pull request Aug 18, 2016

Do not remove unlinted files from cache if --cache is used #6780

Closed

IanVS removed the do not merge This pull request should not be merged yet label Aug 18, 2016

royriojas force-pushed the issue6780 branch from c4caf4d to b878d1d Compare August 22, 2016 01:55

vitorbal reviewed Aug 23, 2016
View reviewed changes

New: do not remove non visited files from cache. (fixes eslint#6780)

a6ae71f

royriojas force-pushed the issue6780 branch from b878d1d to a6ae71f Compare August 23, 2016 18:58

ilyavolodin merged commit 21ab784 into eslint:master Aug 24, 2016

sergesemashko mentioned this pull request Jan 27, 2017

Implement caching stylelint/stylelint#2293

Merged

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: do not remove non visited files from cache. (Fixes #6780) #6921

New: do not remove non visited files from cache. (Fixes #6780) #6921

royriojas commented Aug 16, 2016

eslintbot commented Aug 16, 2016

jquerybot commented Aug 16, 2016

eslintbot commented Aug 16, 2016

IanVS commented Aug 17, 2016

royriojas commented Aug 17, 2016

vitorbal commented Aug 17, 2016

IanVS commented Aug 18, 2016

IanVS commented Aug 18, 2016

platinumazure commented Aug 18, 2016

IanVS commented Aug 18, 2016

royriojas commented Aug 18, 2016

eslintbot commented Aug 22, 2016

royriojas commented Aug 22, 2016

IanVS commented Aug 23, 2016

platinumazure commented Aug 23, 2016

IanVS commented Aug 23, 2016 •

edited

platinumazure commented Aug 23, 2016

IanVS commented Aug 23, 2016

vitorbal Aug 23, 2016

vitorbal Aug 23, 2016 •

edited

eslintbot commented Aug 23, 2016

royriojas commented Aug 23, 2016 •

edited

ilyavolodin commented Aug 23, 2016

vitorbal commented Aug 24, 2016

vitorbal commented Aug 24, 2016 •

edited

New: do not remove non visited files from cache. (Fixes #6780) #6921

New: do not remove non visited files from cache. (Fixes #6780) #6921

Conversation

royriojas commented Aug 16, 2016

eslintbot commented Aug 16, 2016

jquerybot commented Aug 16, 2016

eslintbot commented Aug 16, 2016

IanVS commented Aug 17, 2016

royriojas commented Aug 17, 2016

vitorbal commented Aug 17, 2016

IanVS commented Aug 18, 2016

IanVS commented Aug 18, 2016

platinumazure commented Aug 18, 2016

IanVS commented Aug 18, 2016

royriojas commented Aug 18, 2016

eslintbot commented Aug 22, 2016

royriojas commented Aug 22, 2016

IanVS commented Aug 23, 2016

platinumazure commented Aug 23, 2016

IanVS commented Aug 23, 2016 • edited

platinumazure commented Aug 23, 2016

IanVS commented Aug 23, 2016

vitorbal Aug 23, 2016

Choose a reason for hiding this comment

vitorbal Aug 23, 2016 • edited

Choose a reason for hiding this comment

eslintbot commented Aug 23, 2016

royriojas commented Aug 23, 2016 • edited

ilyavolodin commented Aug 23, 2016

vitorbal commented Aug 24, 2016

vitorbal commented Aug 24, 2016 • edited

IanVS commented Aug 23, 2016 •

edited

vitorbal Aug 23, 2016 •

edited

royriojas commented Aug 23, 2016 •

edited

vitorbal commented Aug 24, 2016 •

edited