Closed Bug 1246896 Opened 4 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: pbro, Assigned: standard8)

References

Details

Attachments

(3 files)

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.
Component: General → ESLint
Duplicate of this bug: 1342778
See Also: → 1245779
Priority: -- → P3
Assignee: nobody → 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 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.
Attachment #8922856 - Flags: review?(florian) → review+
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
Attachment #8922855 - Flags: review?(florian) → review+
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.
Attachment #8922857 - Flags: review?(florian) → review+
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.
Pushed by mbanner@mozilla.com:
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
Blocks: 1412778
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.