Closed Bug 1186834 Opened 4 years ago Closed 4 years ago
we should run flake8 automatically on harness for talos
40 bytes, text/x-review-board-request
40 bytes, text/x-review-board-request
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.
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 :)
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.
lets see what impact it has on each job, I assume it will be good. Will it work on all platforms?
Note that mozreview should support some form of linting at review time, whenever we get talos in tree...
(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.
So, following the discussion in bug 1225553, let's move on with this!
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Depends on: 1226841
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.
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.
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.
Bug 1186834 - run flake8 automatically on harness for talos. r=jmaher
Attachment #8691875 - Flags: review?(jmaher)
Attachment #8691874 - Flags: review?(jmaher) → review+
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.
Attachment #8691875 - Flags: review?(jmaher) → review+
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.
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
You need to log in before you can comment on or make changes to this bug.