Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

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.

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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.

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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

	Mark Banner (:standard8)
	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

	David Burns :automatedtester
	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`?

	Mark Banner (:standard8)
	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

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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

	David Burns :automatedtester
	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.

	Andreas Tolfsen ❲:ato❳ 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.

	Andreas Tolfsen ❲:ato❳ 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.

	Andreas Tolfsen ❲:ato❳ 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.

	Pulsebot
	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

	Carsten Book [:Tomcat]
	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