Add ESLint static analysis mozreviewbot

RESOLVED FIXED

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Details

Attachments

(4 attachments)

Bug 1196263 added infrastructure for creating automated static analysis bots that will post review to MozReview.

We should get an ESLint bot in there, especially given the recent activity around ESLint in various mailing list threads[1] and the tree[2].

[1]: See these threads for context:
https://mail.mozilla.org/pipermail/firefox-dev/2015-November/003557.html
https://mail.mozilla.org/pipermail/firefox-dev/2015-November/003559.html
https://mail.mozilla.org/pipermail/firefox-dev/2015-November/003594.html

[2]: Bug 1228628, for example.
See also bug 875605 for a c++ linter.
Assignee

Updated

4 years ago
Depends on: 1229113
Bug 1229106 - mozreviewbots: add ESLintBot (bug 1229106) r=dminor,smacleod
Attachment #8697102 - Flags: review?(smacleod)
Attachment #8697102 - Flags: review?(dminor)
Bug 1229106 - mozreviewbots: add some tests for ESLintBot (bug 1229106) r=dminor,smacleod
Attachment #8697103 - Flags: review?(smacleod)
Attachment #8697103 - Flags: review?(dminor)
Gijs and I just hacked this up during Mozlando.
Assignee: nobody → mconley
Comment on attachment 8697102 [details]
MozReview Request: Bug 1229106 - mozreviewbots: add ESLintBot (bug 1229106) r=dminor,smacleod

https://reviewboard.mozilla.org/r/27505/#review24883

lgtm

::: pylib/mozreviewbots/eslintbot/__main__.py:28
(Diff revision 1)
> +    """This bot runs flake8 against python files under review"""

Please update docstring

::: pylib/mozreviewbots/eslintbot/__main__.py:36
(Diff revision 1)
> +        self.logger.info('landing_repo_url: %s - repo_url: %s - revision: %s'

This should probably be a logger.debug (if it is needed at all)

::: pylib/mozreviewbots/eslintbot/__main__.py:45
(Diff revision 1)
> +                             % (revision, ESLINT_CONFIG))

Return here to avoid extra work.

::: pylib/mozreviewbots/eslintbot/__main__.py:68
(Diff revision 1)
> +                ('--output-file=%s' % OUTPUT_FILE)

I believe you could use tempfile.NamedTemporaryFile here rather than having a hard coded path, assuming we're planning to only run the bot on unix like systems.
Attachment #8697102 - Flags: review?(dminor) → review+
Comment on attachment 8697103 [details]
MozReview Request: Bug 1229106 - mozreviewbots: add some tests for ESLintBot (bug 1229106) r=dminor,smacleod

https://reviewboard.mozilla.org/r/27507/#review24885

lgtm

::: pylib/mozreviewbots/tests/test-eslintbot.t:7
(Diff revision 1)
> +  $ cat > eslintbot.ini << EOF

It might be worth breaking this out into a helper function like was done for the pylintbot in case we add more tests in the future.
Attachment #8697103 - Flags: review?(dminor) → review+

Comment 7

4 years ago
Is there a bug on file about the node/eslint requirements for the machines that run this and/or the infra part of standing this up, once smacleod also reviews? If not, where/what would we need to move this forward? :-)
Flags: needinfo?(dminor)
Hi Gijs, I've filed Bug 1232626 for creating an AWS instance on which to run this bot (I'll create one for now, I can add others if it can't keep up with its reviews.)

If you have any specific requirements, please comment over there.
Flags: needinfo?(dminor)

Updated

4 years ago
Attachment #8698576 - Flags: review?(dminor)

Updated

4 years ago
Attachment #8698575 - Flags: review?(dminor)
Comment on attachment 8697102 [details]
MozReview Request: Bug 1229106 - mozreviewbots: add ESLintBot (bug 1229106) r=dminor,smacleod

Clearing this review flag, I'll go ahead with any feedback I have when finishing up the pulse messaging stuff.
Attachment #8697102 - Flags: review?(smacleod)
Attachment #8697103 - Flags: review?(smacleod)

Comment 12

4 years ago
Is this ready to land?
Flags: needinfo?(mconley)
It actually already did land:

https://hg.mozilla.org/hgcustom/version-control-tools/rev/e632eedaac18
https://hg.mozilla.org/hgcustom/version-control-tools/rev/a310bc7e6842

But there are still some missing pieces. We need some Pulse messages to kick off the bot, and we also need to make sure that the bot only comments on lines that were changed by the patch (which, I don't believe, it currently does).

So we can probably close this out and file the new ones.

I'll create a new meta bug for the ESLint mozreviewbot, and close this out.
No longer blocks: eslint
No longer depends on: 1229113, 1196263
Flags: needinfo?(mconley)
Assignee

Updated

4 years ago
Blocks: 1239044
Assignee

Updated

4 years ago
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.