1777527 - browser/components/newtab tests suddenly lint node_modules

Status: RESOLVED DUPLICATE of bug 1648286

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

Description

[Emily McMinn :emcminn]

Steps to reproduce: On an updated mozilla-central, run ./mach npm test --prefix=browser/components/newtab

expected result: npm unit tests run (and pass)

actual result: the tests fail at the npm-run-all lint step; the failures are caused by linting being run on node_modules, which should be excluded.

A git bisect shows https://bugzilla.mozilla.org/show_bug.cgi?id=1776104 to be the regressing bug; after playing around it seems like the changes to Generated.txt in particular are the problem.

I'm not sure if that change should be reverted, or if we should be updating browser/components/newtab itself, can someone weigh in?

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1776104

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:ryanvm, since you are the author of the regressor, bug 1776104, could you take a look?
For more information, please visit auto_nag documentation.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

I'm on PTO for the next week, so I can't look at this until I'm back. In the first patch from that bug, I added an Unvalidated.txt file that can be used for directories that need exempting from the new existence checks due to edge cases like these. We could probably add a node_modules exception to it there and hook that into the linting code to resolve this bug. Sorry for the hassle :(

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

https://phabricator.services.mozilla.com/D150070 caused this; I suspect the right fix would to revert the removal of lines containing node-modules from tools/rewriting/Generated.txt. So that they don't inadvertently get removed again, we'd probaby want to add a comment explaining that even though those files are not checked into the tree, they are required to be installed locally in some developers' trees, and shouldn't be linted.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

They are "generated" by the developer locally running ./mach npm install. That said, putting them in Unvalidated.txt could be fine too; I dunno if Ryan has a preference here.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Or, as Emily pointed out, elsewhere, it seems like it wouldn't be unreasonable to add all the files (except maybe the ones in dom/tests?) that were removed back somewhere. I'd be somewhat inclined to leave them in Generated, as they are not actually hand-written files, but really are generated.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Needinfo to odvarko just so he sees this as it may be causing problems for devtools too. (Though your thoughts on it are more than welcome!)

Comment 8

[Mark Banner (:standard8)]

The top-level .eslintignore automatically ignores node_modules across the tree. Since newtab still has its own setup, maybe it needs a .eslintignore as well?

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yes, node-modules dirs in devtools shouldn't be linted and I agree with adding back node-modules to Generated.txt.
However, having properly working .eslintignore to ignore node_modules across the tree sounds safer to me.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Adding it back to Generated.txt won't work by itself. It'll cause all builds in CI to break since that directory doesn't exist there when the clang static-analysis plugin runs (see what the patch in the regressing bug actually does). Strongly agree that fixing this on the .eslintignore side of things seems like the better fix here (not to mention more expedient if it can't wait until I'm back next week).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Bug 1776104 was backed out for some other issues. I'll take a look at making the .eslintignore changes before re-landing.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

The failures here are all with the license linter. I've confirmed locally that this can be worked around by adding a '**/node_modules/' rule to the exclude section of tools/lint/license.yml. With that added, ./mach npm test --prefix=browser/components/newtab runs without issue.

What's interesting to me is that it appears that we have a rule in https://searchfox.org/mozilla-central/source/tools/lint/mach_commands.py#32 that appears to be trying to exclude node_modules from linters. And maybe that's actually working in CI given that the license job was green there when the regressing patches landed? But why wouldn't it apply to local runs also?

Comment 13

[Mark Banner (:standard8)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)

What's interesting to me is that it appears that we have a rule in https://searchfox.org/mozilla-central/source/tools/lint/mach_commands.py#32 that appears to be trying to exclude node_modules from linters. And maybe that's actually working in CI given that the license job was green there when the regressing patches landed? But why wouldn't it apply to local runs also?

The linters only checkout the necessary source - we only install the various node_modules directories for the builders that require them. The exclude section seems to be matching the top level node_modules only.

We've hit this before with other linters (bug 1648286), I've just attached a patch there that should fix it.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:andi, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 15

[Mark Banner (:standard8)]

Ryan, do you know if this fixed now that bug 1648286 landed?

Comment 16

[Mark Banner (:standard8)]

I believe this is fixed now that bug 164286 landed, please re-open if it is still an issue.