ESLint rule that checks for CPOW usages in tests

RESOLVED FIXED in Firefox 45

Status

()

Firefox
Developer Tools
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pbro, Assigned: miker)

Tracking

unspecified
Firefox 45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 obsolete attachment)

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?
- 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.
Assignee: nobody → mratcliffe
Created 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
Attachment #8681399 - Flags: review?(pbrosset)
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.
Attachment #8681399 - Flags: review?(pbrosset)
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 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
Attachment #8681399 - Flags: review?(pbrosset)
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.
Attachment #8681399 - Flags: review?(pbrosset)
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/
Attachment #8681399 - Flags: review?(pbrosset)
Attachment #8681399 - Flags: review?(pbrosset) → review+
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
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 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/
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/53f9d55997a5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Attachment #8681399 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.