1309963 - eslint config errors are swallowed and mach eslint does not fail

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: WiP - v1

I have some local issue with eslint that causes it to spit out an error message when it is run, complaining about eslint-plugin-mozilla missing.

When running mach eslint though, all I get is: ✖ 0 problems (0 errors, 0 warnings)

Part of the problem is that that the return code is not checked. The first line of output is empty, so the json parsing at https://dxr.mozilla.org/comm-central/source/mozilla/tools/lint/eslint.lint#334 succeeds and no errors are found.

The attached patch fixes that and will print the output correctly.

Unfortunately this is not all of the problem. Even with this issue, the return code of mach eslint is 0. Returning 1 does not trigger any errors, and looking at https://dxr.mozilla.org/comm-central/source/mozilla/python/mozlint/mozlint/roller.py#47 returning a number will indeed just ignore the result.

I unfortunately don't have time at the moment to check how to best make mach fail correctly, but I trust someone will create an awesome solution based on this patch.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1309963 - Fix hidden eslint error output when the first line is blank,

The 'eslint' linter will assume there is an error message if the output is not valid
JSON. However, if output[0] evaluates to False, then it will also replace the output
with '[]' (which means success).

But if the error starts with a blank line, then output[0] is False and we wrongfully
don't print the rest of the message.

Review commit: https://reviewboard.mozilla.org/r/85650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85650/

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1309963 - Make sure return codes are passed from .lint files up to the cli,

If a linter returns a status code instead of a list of results, mozlint will throw away
that status code and move on to the next linter. We should make sure that status code
bubbles all the way up to the cli.

Review commit: https://reviewboard.mozilla.org/r/85652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85652/

Comment 3

[James Graham [:jgraham]]

Comment on attachment 8800832 [details]
Bug 1309963 - Make sure return codes are passed from .lint files up to the cli,

https://reviewboard.mozilla.org/r/85652/#review85096

::: python/mozlint/mozlint/roller.py:148
(Diff revision 1)
> +        self.return_code = 0
>          for worker in workers:
>              # parent process blocks on worker.get()
> -            for k, v in worker.get().iteritems():
> +            results, return_code = worker.get()
> +            if results or return_code:
> +                self.return_code = 1

Are we OK with collapsing all other return codes down onto return code 1?

Comment 4

[James Graham [:jgraham]]

Comment on attachment 8800831 [details]
Bug 1309963 - Fix hidden eslint error output when the first line is blank,

https://reviewboard.mozilla.org/r/85650/#review85100

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8800832 [details]
Bug 1309963 - Make sure return codes are passed from .lint files up to the cli,

https://reviewboard.mozilla.org/r/85652/#review85096

> Are we OK with collapsing all other return codes down onto return code 1?

I think so, otherwise we need to solve the problem of what happens if two linters return two different error codes. Seeing as we currently ignore return codes altogether and so far we control what the return codes mean (we don't bubble the eslint/flake8 return codes up), I'm happy to either make them a binary pass/fail or punt on it until we start taking eslint/flake8 into account.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e234deee964
Fix hidden eslint error output when the first line is blank, r=jgraham
https://hg.mozilla.org/integration/autoland/rev/01cef2481413
Make sure return codes are passed from .lint files up to the cli, r=jgraham

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e234deee964
https://hg.mozilla.org/mozilla-central/rev/01cef2481413