Closed
Bug 1376128
Opened 7 years ago
Closed 7 years ago
Enable eslint for Marionette
Categories
(Remote Protocol :: Marionette, enhancement)
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
Details
Attachments
(15 files)
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
standard8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
standard8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
We want to enable linting of the Marionette server code, and eslint is fairly well-established in mozilla-central these days. We want to provide a specialisation of the default ruleset in mozilla-central that is more strict and in line with the current coding style in tsting/marionette.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8881879 [details] Bug 1376128 - Add eslint rules for testing/marionette; https://reviewboard.mozilla.org/r/152932/#review158150
Attachment #8881879 -
Flags: review?(dburns) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8881880 [details] Bug 1376128 - Update lint ignore patterns for testing/marionette; https://reviewboard.mozilla.org/r/152934/#review158154
Attachment #8881880 -
Flags: review?(dburns) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8881881 [details] Bug 1376128 - Use selective imports from error module; https://reviewboard.mozilla.org/r/152936/#review158156 ::: testing/marionette/cookie.js:12 (Diff revision 1) > const {interfaces: Ci, utils: Cu} = Components; > > Cu.import("resource://gre/modules/Services.jsm"); > > Cu.import("chrome://marionette/content/assert.js"); > -Cu.import("chrome://marionette/content/error.js"); > +const {error, InvalidCookieDomainError} = This does not match the style that you used in the other files. ``` const { NoSuchWindowError, InvalidCookieDomainError, } = Cu.import("chrome://marionette/content/error.js", {}); ``` ::: testing/marionette/element.js:14 (Diff revision 1) > Cu.import("resource://gre/modules/Log.jsm"); > > Cu.import("chrome://marionette/content/assert.js"); > Cu.import("chrome://marionette/content/atom.js"); > -Cu.import("chrome://marionette/content/error.js"); > +const { > + JavaScriptError, Please can you order this alphabetically ::: testing/marionette/proxy.js:12 (Diff revision 1) > const {classes: Cc, interfaces: Ci, utils: Cu} = Components; > > Cu.import("resource://gre/modules/Log.jsm"); > Cu.import("resource://gre/modules/Services.jsm"); > > -Cu.import("chrome://marionette/content/error.js"); > +const {error, WebDriverError} = Please split this to be consistent with other files. ::: testing/marionette/server.js:24 (Diff revision 1) > Cu.import("resource://gre/modules/Task.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > Cu.import("chrome://marionette/content/assert.js"); > Cu.import("chrome://marionette/content/driver.js"); > -Cu.import("chrome://marionette/content/error.js"); > +const {error, UnknownCommandError} = Please split this to be consistent with other files ::: testing/marionette/session.js:14 (Diff revision 1) > Cu.import("resource://gre/modules/Log.jsm"); > Cu.import("resource://gre/modules/Preferences.jsm"); > Cu.import("resource://gre/modules/Services.jsm"); > > Cu.import("chrome://marionette/content/assert.js"); > -Cu.import("chrome://marionette/content/error.js"); > +const {error, InvalidArgumentError} = Please split this to be consistent with other files
Attachment #8881881 -
Flags: review?(dburns) → review+
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8881882 [details] Bug 1376128 - Expose unrecognised eslint globals; https://reviewboard.mozilla.org/r/152938/#review158158
Attachment #8881882 -
Flags: review?(dburns) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8881883 [details] Bug 1376128 - Allow duplicated dictionary keys in action module; https://reviewboard.mozilla.org/r/152940/#review158160
Attachment #8881883 -
Flags: review?(dburns) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8881884 [details] Bug 1376128 - Fix inconsistent return type for hasHiddenAttribute; https://reviewboard.mozilla.org/r/152942/#review158166 ::: commit-message-04132:3 (Diff revision 1) > +Bug 1376128 - Fix inconsistent return type for hasHiddenAttribute; r?automatedtester > + > +Returning a value from a finally-block will mask any errors thrown in Not sure I understand this. By doing nothing in the catch we are masking errors as well.
Attachment #8881884 -
Flags: review?(dburns) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8881885 [details] Bug 1376128 - Use consistent return types for accessibility.service; https://reviewboard.mozilla.org/r/152944/#review158168
Attachment #8881885 -
Flags: review?(dburns) → review+
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8881886 [details] Bug 1376128 - Throw RangeError if Window is not found; https://reviewboard.mozilla.org/r/152946/#review158172
Attachment #8881886 -
Flags: review?(dburns) → review+
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8881887 [details] Bug 1376128 - Remove duplicated command entries; https://reviewboard.mozilla.org/r/152948/#review158174
Attachment #8881887 -
Flags: review?(dburns) → review+
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8881879 [details] Bug 1376128 - Add eslint rules for testing/marionette; https://reviewboard.mozilla.org/r/152932/#review158176
Attachment #8881879 -
Flags: review?(standard8) → review+
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8881888 [details] Bug 1376128 - Fix undefined id variable; https://reviewboard.mozilla.org/r/152950/#review158178 ::: commit-message-b2f17:3 (Diff revision 1) > +Bug 1376128 - Fix undefined id variable; r?automatedtester > + > +We try to delete the element etry by "id", which is not defined. is `etry` supposed to be `entry`?
Attachment #8881888 -
Flags: review?(dburns) → review+
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8881880 [details] Bug 1376128 - Update lint ignore patterns for testing/marionette; https://reviewboard.mozilla.org/r/152934/#review158180
Attachment #8881880 -
Flags: review?(standard8) → review+
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8881889 [details] Bug 1376128 - Ensure consistent return types from findElement; https://reviewboard.mozilla.org/r/152952/#review158184 ::: testing/marionette/element.js:463 (Diff revision 1) > - return element.findByXPath(rootNode, startNode, `.//*[@id="${value}"]`); > + let expr = `.//*[@id="${value}"]`; > + return element.findByXPath( rootNode, startNode, expr); > + } > > case element.Strategy.Name: > + { Why do these have `{}` enclosing the body of the case but lower commands don't? seems very inconsistent
Attachment #8881889 -
Flags: review?(dburns) → review+
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8881890 [details] Bug 1376128 - Avoid use of proprietary catch-if statement; https://reviewboard.mozilla.org/r/152954/#review158188
Attachment #8881890 -
Flags: review?(dburns) → review+
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8881891 [details] Bug 1376128 - Use named options for JavaScriptError; https://reviewboard.mozilla.org/r/152956/#review158192
Attachment #8881891 -
Flags: review?(dburns) → review+
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8881892 [details] Bug 1376128 - Remove unused getNavigator_ function from event module; https://reviewboard.mozilla.org/r/152958/#review158194
Attachment #8881892 -
Flags: review?(dburns) → review+
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8881893 [details] Bug 1376128 - Lint testing/marionette; https://reviewboard.mozilla.org/r/152960/#review158202 ::: testing/marionette/capture.js:117 (Diff revision 1) > > let ctx = canvas.getContext(CONTEXT_2D); > if (flags === null) { > flags = ctx.DRAWWINDOW_DRAW_CARET; > - // Disabled in bug 1243415 for webplatform-test failures due to out of view elements. > - // Needs https://github.com/w3c/web-platform-tests/issues/4383 fixed. > + // Disabled in bug 1243415 for webplatform-test > + // failures due to out of view elements. Needs Can you raise a bug to get this reenabled, the github issue appears to have been fixed.
Attachment #8881893 -
Flags: review?(dburns) → review+
Assignee | ||
Comment 33•7 years ago
|
||
(In reply to David Burns :automatedtester from comment #21) > > +Bug 1376128 - Fix inconsistent return type for > > +hasHiddenAttribute; r?automatedtester > > + > > +Returning a value from a finally-block will mask any errors > > +thrown in > > Not sure I understand this. By doing nothing in the catch we are > masking errors as well. I certainly agree with this, but it is clearer to the reader that we are dismissing any error if we explicitly use a catch statement, instead of relying on finally. I would argue that the risk is that someone comes along further down the road and adds a catch statement that returns some alternate value, and then for it not to work because it is being overridden by finally.
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to David Burns :automatedtester from comment #28) > ::: testing/marionette/element.js:463 > (Diff revision 1) > > - return element.findByXPath(rootNode, startNode, `.//*[@id="${value}"]`); > > + let expr = `.//*[@id="${value}"]`; > > + return element.findByXPath( rootNode, startNode, expr); > > + } > > > > case element.Strategy.Name: > > + { > > Why do these have `{}` enclosing the body of the case but lower > commands don't? seems very inconsistent I agree it’s inconsistent. The problem is that the expr variable is being re-defined in the second case because all case statements share the same scope. By using { … } to explicitly create a new scope, we avoid this. However, you are right this is bad and we should fix it. I think this is an argument that the entire function should be refactored. I’m not going to do that in this patch though.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 50•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8881893 [details] Bug 1376128 - Lint testing/marionette; https://reviewboard.mozilla.org/r/152960/#review158202 > Can you raise a bug to get this reenabled, the github issue appears to have been fixed. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1377335.
Comment 51•7 years ago
|
||
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a4aaf6b2ad71 Add eslint rules for testing/marionette; r=automatedtester,standard8 https://hg.mozilla.org/integration/autoland/rev/26ce7fc98bac Update lint ignore patterns for testing/marionette; r=automatedtester,standard8 https://hg.mozilla.org/integration/autoland/rev/9c62ef8b2a61 Use selective imports from error module; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/0ce9f5db98c4 Expose unrecognised eslint globals; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/9136bc55d308 Allow duplicated dictionary keys in action module; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/20fe84e34070 Fix inconsistent return type for hasHiddenAttribute; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/2998fffb6622 Use consistent return types for accessibility.service; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/959a9eb9aff8 Throw RangeError if Window is not found; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/faf51d5fc4c7 Remove duplicated command entries; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/769f3f30cec9 Fix undefined id variable; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/46be98b95ef6 Ensure consistent return types from findElement; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/10b6b2aa53bb Avoid use of proprietary catch-if statement; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/351d17c9722b Use named options for JavaScriptError; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/792219f83f48 Remove unused getNavigator_ function from event module; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/9ede320e5cd2 Lint testing/marionette; r=automatedtester
Comment 52•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a4aaf6b2ad71 https://hg.mozilla.org/mozilla-central/rev/26ce7fc98bac https://hg.mozilla.org/mozilla-central/rev/9c62ef8b2a61 https://hg.mozilla.org/mozilla-central/rev/0ce9f5db98c4 https://hg.mozilla.org/mozilla-central/rev/9136bc55d308 https://hg.mozilla.org/mozilla-central/rev/20fe84e34070 https://hg.mozilla.org/mozilla-central/rev/2998fffb6622 https://hg.mozilla.org/mozilla-central/rev/959a9eb9aff8 https://hg.mozilla.org/mozilla-central/rev/faf51d5fc4c7 https://hg.mozilla.org/mozilla-central/rev/769f3f30cec9 https://hg.mozilla.org/mozilla-central/rev/46be98b95ef6 https://hg.mozilla.org/mozilla-central/rev/10b6b2aa53bb https://hg.mozilla.org/mozilla-central/rev/351d17c9722b https://hg.mozilla.org/mozilla-central/rev/792219f83f48 https://hg.mozilla.org/mozilla-central/rev/9ede320e5cd2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•