1229106 - Add ESLint static analysis mozreviewbot

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Benoit Girard (:BenWa)]

See also bug 875605 for a c++ linter.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1229106 - mozreviewbots: add ESLintBot (bug 1229106) r=dminor,smacleod

Bug 1229106 - mozreviewbots: add ESLintBot (bug 1229106) r=dminor,smacleod

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1229106 - mozreviewbots: add some tests for ESLintBot (bug 1229106) r=dminor,smacleod

Bug 1229106 - mozreviewbots: add some tests for ESLintBot (bug 1229106) r=dminor,smacleod

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Gijs and I just hacked this up during Mozlando.

Comment 5

[Dan Minor [:dminor]]

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.

Comment 6

[Dan Minor [:dminor]]

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.

Comment 7

[:Gijs (he/him)]

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? :-)

Comment 8

[Dan Minor [: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.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1229106 - mozreviewbots: add ESLintBot (bug 1229106) r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27505/diff/1-2/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1229106 - mozreviewbots: add some tests for ESLintBot (bug 1229106) r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27507/diff/1-2/

Comment 11

[Steven MacLeod [:smacleod]]

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.

Comment 12

[:Gijs (he/him)]

Is this ready to land?

Comment 13

[Mike Conley (: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.