Closed Bug 1376128 Opened 2 years ago Closed 2 years ago

Enable eslint for Marionette

Categories

(Testing :: Marionette, enhancement)

Version 3
enhancement
Not set

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: nobody → ato
Status: NEW → ASSIGNED
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 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 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 on attachment 8881882 [details]
Bug 1376128 - Expose unrecognised eslint globals;

https://reviewboard.mozilla.org/r/152938/#review158158
Attachment #8881882 - Flags: review?(dburns) → 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 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 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 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 on attachment 8881887 [details]
Bug 1376128 - Remove duplicated command entries;

https://reviewboard.mozilla.org/r/152948/#review158174
Attachment #8881887 - Flags: review?(dburns) → 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 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 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 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 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 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 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 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+
(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.
(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 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.
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
You need to log in before you can comment on or make changes to this bug.