Open Bug 1397853 Opened 2 years ago Updated 2 months ago

Enable py2 and py3 linter on testing/mochitest


(Testing :: Mochitest, enhancement)

Not set


(Not tracked)


(Reporter: ahal, Unassigned, Mentored)



(Whiteboard: [good-first-bug][lang=py])


(3 files)

+++ 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.
Assignee: nobody → stevea1
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?
Flags: needinfo?(ahalberstadt)
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.
Flags: needinfo?(ahalberstadt)
Depends on: 1398765
You're right, it looks like it's 53 errors. I was confusing it with testing/mozbase which is ~250!
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.
I've made the changes and I ran both:
./mach lint --outgoing
./mach python-test testing/mochitest

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

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]( 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.
Attachment #8911635 - Flags: review?(ahalberstadt)
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 on attachment 8911635 [details]
Bug 1397853 - Enable py2 and py3 linter on testing/mochitest.

Thanks, looks good!
Attachment #8911635 - Flags: review?(ahalberstadt) → review+
Pushed by
Enable py2 and py3 linter on testing/mochitest. r=ahal
Sorry, this had to be backed out for failining linter py-compat at testing/mochitest/

Push with failure:
Failure log:
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/mochitest/ | invalid syntax (is-parseable) 

Please fix the issue and submit and updated patch. Thank you.
Flags: needinfo?(stevea1)
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.
Actually, the other bug is the one that introduced an error. Nice to see that the linter would have been valuable :)
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:
Flags: needinfo?(stevea1)
Attachment #8911635 - Flags: review+ → review?
Attachment #8911635 - Flags: review?
Attachment #8911635 - Flags: review?(ahalberstadt)
Hi Andrew, I've rebased with the latest and then made the fix to testing/mochitest/ But I'm not able to push the change:

pushing to
searching for appropriate review repository
redirecting push to
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.
Flags: needinfo?(ahalberstadt)
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.
Flags: needinfo?(ahalberstadt)
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 on attachment 8915797 [details]
Bug 1397853 - Enable py2 and py3 linter on testing/mochitest.

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.
Attachment #8915797 - Flags: review?(ahalberstadt) → review+
Depends on: 1407769
I was thinking this could be resolved the same as bug 1397849 by adding 'six' as a dependency to the file. But testing/mochitest doesn't have a Would I just create one from scratch? Or is there a template somewhere to start with?
Flags: needinfo?(ahalberstadt)
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 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.
Depends on: 1397849
Flags: needinfo?(ahalberstadt)
Now that bug 1397849 is resolved, what are your thoughts on this one?
Flags: needinfo?(ahalberstadt)
Thanks for the reminder, here's a try run:

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.
Flags: needinfo?(ahalberstadt)
Assignee: stevea1 → nobody
Whiteboard: [good-first-bug] → [good-first-bug][lang=py]
You need to log in before you can comment on or make changes to this bug.