1186834 - we should run flake8 automatically on harness for talos

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Julien Pagès (:parkouss)]

Sometime ago I made a big patch for talos to clean the codebase, making it compliant to pep8 and able to detect some human mistakes (unused variables or such) using flake8.

Now I can see that there are some new issues. This is because we do not run that continuously. So I would like to run this from harness, I don't see what is stopping us - except maybe adding flake8 in the internal pypi repo (but this can be done!)

:jmaher, what do you think about that ? Also, later we could in the same way add Talos unit tests. But this should be done in a followup.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

I like this idea- but the goal of moving into m-c in Q4 makes this a bit harder.  how do you propose we run flake8?  Ideally we will have an automation job that runs like mochitests/talos/etc. for all our harness unittests :)

Comment 2

[Julien Pagès (:parkouss)]

Well I thought about putting it the mozharness talos.py existing file. It will probably be run on every talos job, but it does not take long, and it should be easy to implement.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

lets see what impact it has on each job, I assume it will be good.  Will it work on all platforms?

Comment 4

[William Lachance (:wlach)]

Note that mozreview should support some form of linting at review time, whenever we get talos in tree...

Comment 5

[Julien Pagès (:parkouss)]

(In reply to William Lachance (:wlach) from comment #4)
> Note that mozreview should support some form of linting at review time,
> whenever we get talos in tree...

Well, not anybody uses mozreview. I feel like this is not the job of a review tool (I mean that helps, but ultimately if you want to be sure, this is not the right tool to use).

And then we'll have to find how to run talos unit tests, kind of same story.

Comment 6

[Julien Pagès (:parkouss)]

So, following the discussion in bug 1225553, let's move on with this!

Comment 7

[Julien Pagès (:parkouss)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b078e5a1ba2

Comment 8

[Julien Pagès (:parkouss)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bce180ce09d9

Comment 9

[Julien Pagès (:parkouss)]

So, the first try is broken on purpose - I want to show how it is when the build fails because of issues.

The second try push is the good one. It is based on the patch in bug 1225553 to avoid merge conflict, so I'll wait for bug 1225553 to be landed before asking for a review and landing here.

Comment 10

[Julien Pagès (:parkouss)]

Ok, build failures are fixed now, I retried every talos job in the above try pushes and this looks good.

So, the try push broken on purpose is orange, and in treeherder it says 'flake8 check failed.'. If you open the log (https://treeherder.mozilla.org/logviewer.html#?job_id=14048289&repo=try), the flake8 errors are printed just above the highlighted error line. Looks good to me.

BTW, the build do not fail early - we continue to run talos tests even if flake8 checks are failing. I thought that was the right thing to do - :jmaher, do you agree on that ? Else I can make it fail early.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

interesting question.  I like this idea because then we are not missing data points when this is orange, yet it gives the sheriffs and developers an orange to look at.

Comment 12

[Julien Pagès (:parkouss)]

Attachment: MozReview Request: Bug 1186834 - fix flake8 issues on talos. r=jmaher

Bug 1186834 - fix flake8 issues on talos. r=jmaher

Comment 13

[Julien Pagès (:parkouss)]

Attachment: MozReview Request: Bug 1186834 - run flake8 automatically on harness for talos. r=jmaher

Bug 1186834 - run flake8 automatically on harness for talos. r=jmaher

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8691874 [details]
MozReview Request: Bug 1186834 - fix flake8 issues on talos. r=jmaher

https://reviewboard.mozilla.org/r/26199/#review23575

these changes look great.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8691875 [details]
MozReview Request: Bug 1186834 - run flake8 automatically on harness for talos. r=jmaher

https://reviewboard.mozilla.org/r/26201/#review23577

thanks, this looks simple and reasonable.

Comment 16

[Julien Pagès (:parkouss)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/587e0f970a893362197cb55de2b51d8c51f1193b
Bug 1186834 - fix flake8 issues on talos. r=jmaher

https://hg.mozilla.org/integration/mozilla-inbound/rev/eac92ea657736ebe6d64aa0de12f63d1417f1eb3
Bug 1186834 - run flake8 automatically on harness for talos. r=jmaher

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/587e0f970a89
https://hg.mozilla.org/mozilla-central/rev/eac92ea65773

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0409df514310
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/63c865b224e4