1530409 - ./mach mozlint reports a huge number of defect on JS minified files

Status: RESOLVED WORKSFORME

(Developer Infrastructure :: Lint and Formatting, enhancement, P3)

Description

[Bastien Abadie [:bastien]]

While debugging Relman's code review bot, i found a revision that reports 120k+ Mozlint issues, mostly from a single file in D20888.

Our bot could detect that a huge number of defect is an invalid information, but we would have to determine a maximum issue threshold per file, and that would not be generic.

Mozlint could also detect that the file is minified, skip the normal analysis and simply send an error "Minified file detected".

Comment 1

[Bastien Abadie [:bastien]]

Andrew, what do you think of this issue and how we should react ?

Comment 2

[Andrew Halberstadt [:ahal]]

I sort of think mozlint should stay out of this, since this is an issue specific to eslint/javascript. Maybe our eslint integration can do the detection of minified files, but I'm not really sure how we would do that.. In this specific case, it looks like this babel.js file should have been added to .eslintignore and/or ThirdPartyPaths.txt.

As far as the reviewbot is concerned, I think maybe it would make sense to truncate errors after some number per file (maybe 100? maybe even only 10?). After a certain point, it probably doesn't really make sense to create more phabricator issues as the developer will hopefully get the hint to run mach lint locally anyway.

Comment 3

[Bastien Abadie [:bastien]]

Thank you Andrew, i'll add a check in the code review bot to handle such a case.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

In Lintian (Debian static analyzer), we look for lines with more than 512 chars to detect if the code is minified:
https://salsa.debian.org/lintian/lintian/blob/master/checks/cruft.pm#L39 (yes, this is perl code :)

Comment 5

[Mike Hommey [:glandium]]

Now that we use nodejs for some things, can't we import the actual sources, and minimize them at build time?

Comment 6

[Mark Banner (:standard8)]

(In reply to Mike Hommey [:glandium] from comment #5)

Now that we use nodejs for some things, can't we import the actual sources, and minimize them at build time?

In this case, the developer tools work is importing one of those modules available from node (babel.js). They're actively working on moving over their build system to node, however we also need to be able to vendor node_modules - the processes for that are still in work/discussion.

In any case, third-party imports are always likely to be an issue here, I think limiting to amount of errors is reasonable.

Comment 7

[Bastien Abadie [:bastien]]

We already detect third party files and do not apply any check on them. But this specific file is not in ThirdPartyPaths.txt

Comment 8

[Mark Banner (:standard8)]

As well as ThirdPartyPaths.txt, we now have GeneratedFiles.txt, we also have a list of exclusions in .eslintignore.

For files such as these, they should generally make it into one of those three, so I think there's nothing to do here.