Enable flake8 linter on testing/xpcshell

RESOLVED FIXED in Firefox 57

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ahal, Assigned: stevea1, Mentored)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [good-first-bug])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
To enable this, add 'testing/xpcshell' to the include list in:
tools/lint/flake8.yml

Then run:
./mach lint -l flake8 testing/xpcshell

All the errors will also need to be fixed. If desired, you could look into using the autopep8 module to do this automatically (just be sure to set max-line-length to 99 if you do this).

Be sure to run:
./mach lint --outgoing
./mach python-test testing/xpcshell

to make sure nothing broke.
(Reporter)

Updated

2 years ago
Assignee: nobody → stevea1
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
I've cleaned up all the flake8 errors in the xpcshell files. The python-test ran succesfully.
(Reporter)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8908120 [details]
Bug 1397852 - Enable flake8 linter on testing/xpcshell.

https://reviewboard.mozilla.org/r/179800/#review185152

Thanks, this was really well done. I have few very minor things I'd like you to fix up, once you've pushed a change with these I'll run it through the tests as usual.

::: testing/xpcshell/runxpcshelltests.py:404
(Diff revision 1)
> -                ['-e', 'const _SERVER_ADDR = "localhost"',
> +                          '-e', 'const _SERVER_ADDR = "localhost"',
> -                 '-e', 'const _HEAD_FILES = [%s];' % cmdH,
> +                          '-e', 'const _HEAD_FILES = [%s];' % cmdH,
> -                 '-e', 'const _JSDEBUGGER_PORT = %d;' % dbgport,
> +                          '-e', 'const _JSDEBUGGER_PORT = %d;' % dbgport,

This looks a bit weird, try indenting it four spaces past the return:

    return xpcscmd + [
        '-e', '...',
        '-e', '...',
    ]

::: testing/xpcshell/runxpcshelltests.py:919
(Diff revision 1)
> -          These 3 variables depend on input from the command line and we need to allow for absolute paths.
> +          These 3 variables depend on input from the command line and we need to allow
> +          for absolute paths.
>            This function is overloaded for a remote solution as os.path* won't work remotely.

Please reformat this last line to be part of the above paragraph.

::: testing/xpcshell/runxpcshelltests.py:1376
(Diff revision 1)
>                  test = testClass(test_object,
> -                        verbose=self.verbose or test_object.get("verbose") == "true",
> +                                 verbose=self.verbose or
> +                                 test_object.get("verbose") == "true",
> -                        usingTSan=usingTSan,
> +                                 usingTSan=usingTSan,
> -                        mobileArgs=mobileArgs, **kwargs)
> +                                 mobileArgs=mobileArgs, **kwargs)

This works, but I'm not crazy about the condition being split across two lines. Try something like:

    test = testClase(
        test_object,
        verbose=self.verbose or test_object.get(...
        ...
    )

::: testing/xpcshell/runxpcshelltests.py:1579
(Diff revision 1)
>          self.log.info("INFO | Passed: %d" % self.passCount)
>          self.log.info("INFO | Failed: %d" % self.failCount)
>          self.log.info("INFO | Todo: %d" % self.todoCount)
>          self.log.info("INFO | Retried: %d" % len(self.try_again_list))
>  
> -        if gotSIGINT and not keepGoing:
> +        if gotSIGINT and not keep_going:

Nice, looks like you found + fixed a legit bug!
Attachment #8908120 - Flags: review?(ahalberstadt) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
Ok, I tweaked the formatting for those 3 items. And, yes, the keepGoing thing surprised me when I realized the code could never have worked as written, but I was able to sort out what the original developer intended and fix it. So that was cool.

Comment 6

2 years ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f400f13824d9
Enable flake8 linter on testing/xpcshell. r=ahal

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f400f13824d9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.