Closed Bug 1428585 Opened 2 years ago Closed 2 years ago

Many lint tools do not check exit code

Categories

(Firefox Build System :: Lint and Formatting, enhancement)

enhancement
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: gsnedders, Assigned: jgraham)

Details

Attachments

(1 file)

Found via bug 1428582, which investigating found that the wpt lint has been exiting abnormally (with a failing assertion) for who-knows-how-long.

It seems that the wpt lint, and several others, don't seem to check the exit code of the called subprocess, which seems bad.
I wonder if this (in the WPT case) is the explanation for https://github.com/w3c/web-platform-tests/issues/8271 (Gecko exported tests sometimes contain lint errors)? We have bug 1428582 and quite probably other bugs where the lint throws an exception and hence doesn't run to completion, and nobody notices that this happens?
Do you have some example pushes where this happens? I can try to add a check for the lint exit code, but having some revision that fails to work against would help to ensure I atually fixed the prblem fully.
Flags: needinfo?(geoffers+mozilla)
If you don't have the patch for bug 1428582 and wpt lint has any files to lint it will raise an AssertionError.
Flags: needinfo?(geoffers+mozilla)
Comment on attachment 8941037 [details]
Bug 1428585 - Fail the wpt lint if the lint process has a non-zero exit code,

https://reviewboard.mozilla.org/r/211334/#review217102

::: tools/lint/wpt/wpt.py:31
(Diff revision 1)
> +    if files == [tests_dir]:
> +        print >> sys.stderr, "No files specified, running the full wpt lint (this is slow)"
> +        files = ["--all"]

Was this case actually happening? I think mozlint should prevent running this linter in the first place if there were no wpt related paths being linted.

::: tools/lint/wpt/wpt.py:42
(Diff revision 1)
> +        if proc.returncode != 0:
> +            results.append(result.from_config(config, message="Lint process failed"))

Alternatively you could return the returncode and mozlint will pick that up as a failure, but then if you had legit results to show, they would be lost. So this is probably better.

Maybe at least log the return code in the message.
Attachment #8941037 - Flags: review?(ahalberstadt) → review+
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/849d7fde04aa
Fail the wpt lint if the lint process has a non-zero exit code, r=ahal
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83bfdce958e7
Fixup flake8 error, r=me ON A CLOSED TREE
https://hg.mozilla.org/mozilla-central/rev/849d7fde04aa
https://hg.mozilla.org/mozilla-central/rev/83bfdce958e7
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Testing → Firefox Build System
Assignee: nobody → james
You need to log in before you can comment on or make changes to this bug.