1246896 - ESLint rule no-cpows-in-tests shouldn't report errors when 'content' is used an object property

Status: RESOLVED FIXED

(Firefox Build System :: Lint and Formatting, defect, P3)

Description

[Patrick Brosset <:pbro>]

This test:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/test/browser_rules_context-menu-show-mdn-docs-02.js

does something like this:
let tooltipDocument = cssDocs.tooltip.content.contentDocument;

where cssDocs.tooltip is an object, and "content" one of its properties.

The no-cpows-in-tests rule unfortunately can't tell the difference between this legitimate use of the javascript language and a CPOW usage. It thinks "content" here corresponds to the content window.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1246896 - Add some basic tests for ESLint rule mozilla/no-cpows-in-tests.js.

Review commit: https://reviewboard.mozilla.org/r/193988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193988/

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1246896 - Improve detection for mozilla/no-cpows-in-tests.js 'content' - only notify when content is used as an identifier.

Review commit: https://reviewboard.mozilla.org/r/193990/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193990/

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1246896 - Bump version number of eslint-plugin-mozilla, and update its dev dependencies.

Review commit: https://reviewboard.mozilla.org/r/193992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193992/

Comment 5

[Mark Banner (:standard8)]

Short extra note about the changes:

- I've tried to reduce the duplicates we were getting.
- I covered a few more valid & invalid cases that comment 0, that I found after running on the whole tree.
- no-cpows-in-tests isn't enabled on the whole tree yet, but it is used in quite a few places. I'll file a bug to expand usage once this lands.

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8922856 [details]
Bug 1246896 - Improve detection for mozilla/no-cpows-in-tests.js 'content' - only notify when content is used as an identifier.

https://reviewboard.mozilla.org/r/193990/#review199080

I think this is fine, so r=me, but feel free to re-request review if you make significant changes based on the following comment.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js:20
(Diff revision 1)
>  var helpers = require("../helpers");
>  
> +// Note: we match to the end of the string as well as the beginning, to avoid
> +// multiple reports from MemberExpression statements.
>  var cpows = [
> -  /^gBrowser\.contentWindow/,
> +  /^gBrowser\.contentWindow$/,

I think this is going to miss plenty of cases (eg. let browser = gBrowser.selectedBrowser; browser.contentDocument...). Am I understanding correctly that this is intentional because we prefer to have false negatives rather than annoying false positives?

Not really something you are modifying here, but I'm wondering if merging all these regexps into a single one would significantly improve the performance of this custom eslint rule.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8922855 [details]
Bug 1246896 - Add some basic tests for ESLint rule mozilla/no-cpows-in-tests.js.

https://reviewboard.mozilla.org/r/193988/#review199082

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8922857 [details]
Bug 1246896 - Bump version number of eslint-plugin-mozilla, and update its dev dependencies.

https://reviewboard.mozilla.org/r/193992/#review199086

rs=me, I assumed package-lock.json was somehow auto-generated and didn't really review it.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8922856 [details]
Bug 1246896 - Improve detection for mozilla/no-cpows-in-tests.js 'content' - only notify when content is used as an identifier.

https://reviewboard.mozilla.org/r/193990/#review199080

> I think this is going to miss plenty of cases (eg. let browser = gBrowser.selectedBrowser; browser.contentDocument...). Am I understanding correctly that this is intentional because we prefer to have false negatives rather than annoying false positives?
> 
> Not really something you are modifying here, but I'm wondering if merging all these regexps into a single one would significantly improve the performance of this custom eslint rule.

I think at the moment, it is important to improve what we've got. There's many false positives/duplicates that this is fixing, and we've not yet got this rule applying to the whole tree.

Without complex parsing, we're not going to be able to handle the type of case that you're suggesting, unless we simplify it in some way. We can probably do it, but I'd rather get these simple cases handled first and start getting developers aware of some of the usages that aren't allowed.

We can probably make some improvements, but I think it needs a discussion with those who understand the existing CPOW APIs and the APIs on gBrowser etc - so we can get a full list, and then develop a strategy to start eliminating these.

Re the merging - I'd prefer to keep them separate as that simplifies understanding the regexps. I'll take a look at performance as we start to roll it out the tree, but I suspect it isn't too bad in comparison to some of our other rules.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/015f31e71d70
Add some basic tests for ESLint rule mozilla/no-cpows-in-tests.js. r=florian
https://hg.mozilla.org/integration/autoland/rev/466c9bb6604d
Improve detection for mozilla/no-cpows-in-tests.js 'content' - only notify when content is used as an identifier. r=florian
https://hg.mozilla.org/integration/autoland/rev/2ac044bc1064
Bump version number of eslint-plugin-mozilla, and update its dev dependencies. r=florian

Comment 11

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/015f31e71d70
https://hg.mozilla.org/mozilla-central/rev/466c9bb6604d
https://hg.mozilla.org/mozilla-central/rev/2ac044bc1064