Closed Bug 1397852 Opened 4 years ago Closed 4 years ago
Enable flake8 linter on testing/xpcshell
59 bytes, text/x-review-board-request
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.
Assignee: nobody → stevea1
Status: NEW → ASSIGNED
I've cleaned up all the flake8 errors in the xpcshell files. The python-test ran succesfully.
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+
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.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/f400f13824d9 Enable flake8 linter on testing/xpcshell. r=ahal
You need to log in before you can comment on or make changes to this bug.