Closed Bug 1186834 Opened 4 years ago Closed 4 years ago

we should run flake8 automatically on harness for talos

Categories

(Testing :: Talos, defect)

defect
Not set

Tracking

(firefox45 fixed, b2g-v2.5 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: parkouss, Assigned: parkouss)

References

Details

Attachments

(2 files)

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?
Depends on: 1186844
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.
Flags: needinfo?(jmaher)
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.
Flags: needinfo?(jmaher)
Bug 1186834 - fix flake8 issues on talos. r=jmaher
Attachment #8691874 - Flags: review?(jmaher)
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/mozilla-central/rev/587e0f970a89
https://hg.mozilla.org/mozilla-central/rev/eac92ea65773
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.