1397853 - Enable py2 and py3 linter on testing/mochitest

Status: RESOLVED WORKSFORME

(Testing :: Mochitest, enhancement)

Description

[Andrew Halberstadt [:ahal]]

+++ This bug was initially created as a clone of Bug #1397849 +++

These linters can be enabled by removing 'testing/mochitest' from the exclude section in tools/lint/py2.yml and tools/lint/py3.yml.

You can then see all the errors by running:
./mach lint -l py2 -l py3 testing/mochitest

Each of the errors will need to be fixed before this can land.

To test changes, please be sure to run both:
./mach lint --outgoing
./mach python-test testing/mochitest

to be sure nothing else broke. The latter requires a build, alternatively ask me to push this to try.

Comment 1

[Steve Armand]

Attachment: Output of ./mach python-test testing/mochitest

Since this patch will require modification to a few hundred files, I wanted to make sure the tests worked before I got started. I tried:
./mach python-test testing/mochitest

but it didn't get off the ground. Is this to be expected? Or maybe something with my config isn't right?

Comment 2

[Andrew Halberstadt [:ahal]]

Hm, do you have a build? It should work, but I wouldn't spend too much time trying to get it working.. I can push the change to tryserver for you when it's ready.

I forgot to mention that testing/mochitest/pywebsocket is a 3rd party module, so we should continue to exclude it from these linters. However when I tested locally by changing the exclude from testing/mochitest to testing/mochitest/pywebsocket, it looked like those files are still getting linted, meaning there's a bug in here somewhere.

I'd recommend ignoring this bug for now until I have a chance to figure out this bug.

p.s where are you seeing hundreds of files? I'm getting ~50 errors including the pywebsocket ones.

Comment 3

[Steve Armand]

You're right, it looks like it's 53 errors. I was confusing it with testing/mozbase which is ~250!

Comment 4

[Andrew Halberstadt [:ahal]]

Yeah, testing/mozbase is a lot of files. If you're tired of this kind of work, feel free to ignore that one :). We can also file individual bugs for each of the mozbase components, though that might be just as much work as just fixing them all.

Comment 5

[Steve Armand]

Attachment: Bug 1397853 - Enable py2 and py3 linter on testing/mochitest.

Review commit: https://reviewboard.mozilla.org/r/183038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183038/

Comment 6

[Steve Armand]

I've made the changes and I ran both:
./mach lint --outgoing
./mach python-test testing/mochitest

Looks good on my end.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8911635 [details]
Bug 1397853 - Enable py2 and py3 linter on testing/mochitest.

https://reviewboard.mozilla.org/r/183038/#review188452

We shouldn't make any changes to the 'pywebsocket' module as that's a 3rd party library that's imported into mozilla-central. I mentioned it in comment 2, but should have made it more clear what I meant (sorry for the unnecessary work). If you have [evolve](https://www.mercurial-scm.org/doc/evolution/) installed you can run:

    $ hg uncommit -I "glob:testing/mochitest/pywebsocket/**"
    $ hg revert -a
    
You'll also need to add `testing/mochitest/pywebsocket` to the exclude lists in py2.yml and py3.yml. The rest looks good though.

Comment 8

[Steve Armand]

Comment on attachment 8911635 [details]
Bug 1397853 - Enable py2 and py3 linter on testing/mochitest.

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

Comment 9

[Steve Armand]

Ok, got it. The uncommit worked as advertised - neat trick! I've pulled back the /pywebsocket files and added it to the exclude lists in py2.yml and py3.yml.

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8911635 [details]
Bug 1397853 - Enable py2 and py3 linter on testing/mochitest.

https://reviewboard.mozilla.org/r/183038/#review189244

Thanks, looks good!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbe60b600dc0
Enable py2 and py3 linter on testing/mochitest. r=ahal

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, this had to be backed out for failining linter py-compat at testing/mochitest/runtests.py:2236:

https://hg.mozilla.org/integration/autoland/rev/16813d52b75cbb18f9f47c48b5c5a30cf02eae0c

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=dbe60b600dc0b9aabce3d197b438e4a6816f117e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133812479&repo=autoland
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/mochitest/runtests.py:2236:22 | invalid syntax (is-parseable) 

Please fix the issue and submit and updated patch. Thank you.

Comment 13

[Andrew Halberstadt [:ahal]]

This passed on try yesterday, I think it conflicted with bug 1403616. Steve, you'll need to rebase on top of the latest central to see this error. No worries you didn't do anything wrong, just bad luck.

Comment 14

[Andrew Halberstadt [:ahal]]

Actually, the other bug is the one that introduced an error. Nice to see that the linter would have been valuable :)

Comment 15

[Andrew Halberstadt [:ahal]]

Also in case you don't see my review comments for your testing/mozbase patch, we'll need to use the six library to fix this. See:
http://python-future.org/compatible_idioms.html#raising-exceptions

Comment 16

[Steve Armand]

Hi Andrew, I've rebased with the latest and then made the fix to testing/mochitest/runtests.py. But I'm not able to push the change:

pushing to https://reviewboard-hg.mozilla.org/autoreview
searching for appropriate review repository
redirecting push to https://reviewboard-hg.mozilla.org/gecko
searching for changes
no changes found
submitting 2 changesets for review
abort: Review request is submitted or discarded.
You must reopen the review request before it can be updated.
Review requests should only be reopened if your changes have not landed or have
been backed out - file new bugs for follow-up work.

I wasn't able to figure out how to "reopen the review request" so I need a hand with that.

Comment 17

[Andrew Halberstadt [:ahal]]

Thanks for the fix! You have to go to the review request in mozreview and look for a gray banner at the top. There's a "Reopen" link. Once you click that you'll be able to push to the review again.

Comment 18

[Steve Armand]

Attachment: Bug 1397853 - Enable py2 and py3 linter on testing/mochitest.

Review commit: https://reviewboard.mozilla.org/r/187018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187018/

Comment 19

[Steve Armand]

Comment on attachment 8911635 [details]
Bug 1397853 - Enable py2 and py3 linter on testing/mochitest.

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

Comment 20

[Steve Armand]

Ok, I've pushed the fix and it looks like it came over in two changesets. Not sure if this is a problem or not...

Comment 21

[Andrew Halberstadt [:ahal]]

Comment on attachment 8915797 [details]
Bug 1397853 - Enable py2 and py3 linter on testing/mochitest.

https://reviewboard.mozilla.org/r/187018/#review192250

Thanks. This might now have the same problem that your mozbase patch did. I'll have to spend some time looking into how we can get six added to the test machines in CI.

Comment 22

[Steve Armand]

I was thinking this could be resolved the same as bug 1397849 by adding 'six' as a dependency to the setup.py file. But testing/mochitest doesn't have a setup.py. Would I just create one from scratch? Or is there a template somewhere to start with?

Comment 23

[Andrew Halberstadt [:ahal]]

Good question. Technically you shouldn't need to do anything. Since mochitest depends on mozbase which in turn depends on `six`, it should be installed wherever mochitest is. It would be good form for mochitest to explicitly declare it needs `six`, though there is currently no mechanism to do this. Mochitest isn't a python package, so we can't use a setup.py. We could maybe create a requirements.txt file, but then nothing would really be using it anyway.

I'd suggest we just leave it. We'll definitely need to wait for bug 1397849 to land though.

Comment 24

[Steve Armand]

Now that bug 1397849 is resolved, what are your thoughts on this one?

Comment 25

[Andrew Halberstadt [:ahal]]

Thanks for the reminder, here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf06ce916401eefbbd5e052b6ede7dcaf2338ae9

It looks like there are two commits with the same message. Could you squash those together? (With either `hg histedit` or `hg fold` if you have the evolve extension enabled). I also noticed some merge conflicts, so you'll have to rebase to the latest central.

Comment 26

[Sumagna Das]

Is there anyone working on this bug?
if not, i would like to try.

Comment 27

[Sumagna Das]

i was starting to work on this one but it seems like there is nothing to fix. I removed the line from py2.yml but couldn't find any line containing testing/mochitest. Then executed all the commands stated in the description and it didnt report any errors. So why is this bug still not resolved?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

AFAICT, all 3 python linters we currently run have no exceptions for testing/mochitest (black.yml, flake8.yml, pylint.yml). I think we can close this.