1218412 - ESLint rule that checks for CPOW usages in tests

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Brosset <:pbro>]

We've removed a lot of CPOW usage from our devtools mochitests, but we still have some. For sure we should not create new tests that directly access 'content', that's why I think a simple ESLint rule could help.

'content' is one way to access the content directly via a CPOW in a test, are there other ways?

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

- gBrowser.contentWindow
- gBrowser.contentDocument
- gBrowser.selectedBrowser.contentWindow
- browser.contentDocument
- window.content
- content

We also need to have this work in this kind of situation:
```
let b = gBrowser;
b.contentWindow...
```

With that in mind just matching the rightmost identifier is probably best so that leaves:
- contentWindow
- contentDocument
- content

I *think* that is all we would need to match.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: MozReview Request: Bug 1218412 - Create ESLint rule to check for CPOWS in browser mochitests r?=pbrosset

Bug 1218412 - Create ESLint rule to check for CPOWS in browser mochitests r?=pbrosset

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8681399 [details]
MozReview Request: Bug 1218412 - Create ESLint rule to check for CPOWS in browser mochitests r?=pbrosset

https://reviewboard.mozilla.org/r/23825/#review21349

Could you please find a way to only warn if content or contentDocument are *not* used as properties of a MemberExpression?

::: testing/eslint-plugin-mozilla/docs/no-cpows-in-tests.rst:10
(Diff revision 1)
> +Prevent access to CPOWs in browser mochitests.

Can you please add to this doc information about how the rule detects a file is a browser mochitest and how it detects CPOW usage.

::: testing/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js:14
(Diff revision 1)
> +var helpers = require("../helpers");

helpers isn't used here, please remove this import.

::: testing/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js:23
(Diff revision 1)
> +    Identifier: function(node) {

I wonder if using `Identifier` here is correct. For example, the rule will warn about this too:

`let tabs = addonDebugger.frame.contentDocument.getElementById("toolbox-tabs").children;`

because it contains the identifier `contentDocument`, however this is *not* a CPOW usage.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://reviewboard.mozilla.org/r/23825/#review21349

> I wonder if using `Identifier` here is correct. For example, the rule will warn about this too:
> 
> `let tabs = addonDebugger.frame.contentDocument.getElementById("toolbox-tabs").children;`
> 
> because it contains the identifier `contentDocument`, however this is *not* a CPOW usage.

Yes, we will get false negatives but there is no way from the AST to detect whether we are trying to access content directly or not.

What we could do is look for the phrases I list in the bug... that way we may miss some but it will be less annoying.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8681399 [details]
MozReview Request: Bug 1218412 - Create ESLint rule to check for CPOWS in browser mochitests r?=pbrosset

Bug 1218412 - Create ESLint rule to check for CPOWS in browser mochitests r?=pbrosset

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8681399 [details]
MozReview Request: Bug 1218412 - Create ESLint rule to check for CPOWS in browser mochitests r?=pbrosset

https://reviewboard.mozilla.org/r/23825/#review22055

I've tested it with a test from our codebase, and it appeared to work well: \devtools\client\debugger\test\mochitest\browser_dbg_break-on-dom-event-01.js

However, running it on the following sample code produced false positive, and I think there might be ways to fix this:

```
// All of these lines produce warnings, and that's fine:
content.document.querySelector("input");
is(content.wrappedJSObject.foobar);
EventUtils.synthesizeKey("e", { shiftKey: 1 }, content);

// But these lines also produce warnings, and these are false positives:
test.content = 2;
var content = {a: 1};
content.a;
```

When content appears as the property of an object (`test.content`) then I think we should have enough info in the AST to not warn about it (unless this object is `window`.
When content is used as a variable declaration `var content`, then I think it should be easy to skip this too.
I think not warnings against `content.a` however would be hard, and I think that's fine, we can leave this as is, I don't see a good reason for someone to declare a local variable named `content`.

::: testing/eslint-plugin-mozilla/docs/index.rst:24
(Diff revision 2)
> +- "gBrowser.contentWindow"

Do you think we should include so much doc in the index.rst? I thought just a one-liner summary would be fine, and then keep the details for the rule's rst file.

::: testing/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js:49
(Diff revision 2)
> +        if (expression.startsWith(cpow)) {

Running the rule locally fails for me, on this line and a couple of other lines below. They all use startsWith but string.prototype.startsWith was only added in ES6 (I think). Is there a way to execute rules with ES6?

I've had to replace those with 
expression.substring(0, cpow.length) === cpow;
for it to work.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8681399 [details]
MozReview Request: Bug 1218412 - Create ESLint rule to check for CPOWS in browser mochitests r?=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23825/diff/2-3/

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8681399 [details]
MozReview Request: Bug 1218412 - Create ESLint rule to check for CPOWS in browser mochitests r?=pbrosset

https://reviewboard.mozilla.org/r/23825/#review22397

::: testing/eslint-plugin-mozilla/lib/helpers.js:194
(Diff revision 3)
> +  getIsGlobalScope: function(context) {

Please add a jsdoc comment for this new function

::: testing/eslint-plugin-mozilla/lib/helpers.js:205
(Diff revision 3)
> +  getIsBrowserMochitest: function(scope) {

Please add a jsdoc comment for this new function

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://reviewboard.mozilla.org/r/23825/#review22055

> Running the rule locally fails for me, on this line and a couple of other lines below. They all use startsWith but string.prototype.startsWith was only added in ES6 (I think). Is there a way to execute rules with ES6?
> 
> I've had to replace those with 
> expression.substring(0, cpow.length) === cpow;
> for it to work.

Looking at http://jsperf.com/string-startswith/3 using a regex is over twice as fast and arguably easier to read so I have used this method.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8681399 [details]
MozReview Request: Bug 1218412 - Create ESLint rule to check for CPOWS in browser mochitests r?=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23825/diff/3-4/

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/53f9d55997a5

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/53f9d55997a5