Closed Bug 1300776 Opened 3 years ago Closed 3 years ago

Add testing/marionette/harness to flake8 linter (except mixins, tests)

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

Details

(Keywords: pi-marionette-harness-tests)

Attachments

(1 file)

We should add the main Marionette harness files to the linter. It's a hassle right now to see all those failures, which can let you slip through some failures in your own patches.

It was not that much work for the core files, but mixins and tests might be something we can do later due to massive amount of failures.
Comment on attachment 8788455 [details]
Bug 1300776 - Add testing/marionette/harness to flake8 linter (except mixins, tests) .

https://reviewboard.mozilla.org/r/76958/#review75172

::: testing/marionette/harness/.flake8:3
(Diff revision 1)
> +[flake8]
> +max-line-length = 99
> +exclude = __init__.py,disti/*,build/*,session/*,marionette/runner/mixins/*, marionette/tests/*

I think you'll need to add these to the flake8.lint file instead:
https://dxr.mozilla.org/mozilla-central/source/tools/lint/flake8.lint#138

See https://bugzilla.mozilla.org/show_bug.cgi?id=1277851 for more information. I put a hack to support subdirectory .flake8 files a little bit, but it only works if the exact directory containing the .flake8 file is passed in. I.e |mach lint testing/marionette/harness| will pick this config up, whereas |mach lint testing/marionette| will not.

I also filed https://gitlab.com/pycqa/flake8/issues/143 to fix this properly upstream. I wouldn't block on that though.
Comment on attachment 8788455 [details]
Bug 1300776 - Add testing/marionette/harness to flake8 linter (except mixins, tests) .

https://reviewboard.mozilla.org/r/76958/#review75174

Lgtm once exclude paths moved to flake8.lint.
Attachment #8788455 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8788455 [details]
Bug 1300776 - Add testing/marionette/harness to flake8 linter (except mixins, tests) .

https://reviewboard.mozilla.org/r/76958/#review75172

> I think you'll need to add these to the flake8.lint file instead:
> https://dxr.mozilla.org/mozilla-central/source/tools/lint/flake8.lint#138
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1277851 for more information. I put a hack to support subdirectory .flake8 files a little bit, but it only works if the exact directory containing the .flake8 file is passed in. I.e |mach lint testing/marionette/harness| will pick this config up, whereas |mach lint testing/marionette| will not.
> 
> I also filed https://gitlab.com/pycqa/flake8/issues/143 to fix this properly upstream. I wouldn't block on that though.

Well the f8 job passed successfully and also locally the exlude dir works when put in the specific marionette related subdir config file. But I also see the point with the global config file, because there we can see which folders aren't covered yet!
Oh sorry, you're right. This works because we are passing in 'testing/marionette/harness' directly from the include section in flake.lint. For some reason I thought we were passing in just 'testing/marionette'. I'll leave it up to you then, feel free to close the issue and land as is. Just be aware that this will make 'testing/marionette/harness' run in a subprocess:
https://dxr.mozilla.org/mozilla-central/source/tools/lint/flake8.lint#110
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fafc28ee1d5d
Add testing/marionette/harness to flake8 linter (except mixins, tests) . r=ahal
https://hg.mozilla.org/mozilla-central/rev/fafc28ee1d5d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.