Closed Bug 1264063 Opened 4 years ago Closed 4 years ago

Disable noisy eslint rules untils they get fixed

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: ochameau, Assigned: pbro)

References

Details

Attachments

(2 files, 4 obsolete files)

There is so many warnings now that we don't even see errors in Treeherder shortlog panel...
This is also very disturbing for contributors.
Even mozilla employees contributing to /devtools/ find this disturbing.

We should disable these rules until they get fixed.
Just disable them everywhere it is needed.
Attachment #8740625 - Flags: review?(jryans)
Also fix this rule everywhere in /devtools/.
It is surprising to have this enabled and have so few warnings?
It seems to report all ContentTask usages... unless I miss something.
Attachment #8740626 - Flags: review?(jryans)
If we do care about this rule, we should make it an error.
Attachment #8740627 - Flags: review?(jryans)
Comment on attachment 8740625 [details] [diff] [review]
Disable noisy eslint warning in /devtools/

Review of attachment 8740625 [details] [diff] [review]:
-----------------------------------------------------------------

My preference would be to disable rules from devtools/.eslintrc until they can be enabled as an error (2).

In a previous bug, I believe :pbro said there's not much point in configuring rules as warnings, and currently it's only the plugins that do this.  By avoiding warnings altogether, we get out of the bad state in Treeherder: either the rule is off or it's an error and must be fixed.
Attachment #8740625 - Flags: review?(jryans) → review?(pbrosset)
Comment on attachment 8740626 [details] [diff] [review]
Fix the various no-cpow-in-tests warning in /devtools/

Review of attachment 8740626 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't seem right to me.  Should we disable the rule?  Or at least file a bug to fix it with ContentTask?

Mike, what do you think here?
Attachment #8740626 - Flags: review?(jryans) → feedback?(mratcliffe)
Comment on attachment 8740627 [details] [diff] [review]
Toggle no-cpow-in-tests to error to prevent regressing it

Review of attachment 8740627 [details] [diff] [review]:
-----------------------------------------------------------------

Let's see what we determine about the previous no-cpow change.
Attachment #8740627 - Flags: review?(jryans)
Comment on attachment 8740625 [details] [diff] [review]
Disable noisy eslint warning in /devtools/

Review of attachment 8740625 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, I don't see a point of having a rule if it's set to generate warnings. And I've talked to people multiple times about fixing this noise in the past. I entirely agree that this is confusing for everyone and that we should get rid of them. I don't really know why this hasn't been fixed already, especially because it's such a simple change.
What I would suggest here is:
- turn react/sort-comp to 2 (error) and fix the corresponding code, because it's such a simple change
- turn react/prop-types to 0 (disabled) in the main devtools/.eslintrc file because it requires more code and I'm not comfortable with it. I'd like someone working on responsive.html or about:debugging to do it. It should be quick because there aren't too many errors.

I'll upload a patch that does this in a minute.
Attachment #8740625 - Flags: review?(pbrosset)
Attachment #8740625 - Attachment is obsolete: true
Attachment #8740848 - Flags: review?(poirot.alex)
Comment on attachment 8740626 [details] [diff] [review]
Fix the various no-cpow-in-tests warning in /devtools/

Review of attachment 8740626 [details] [diff] [review]:
-----------------------------------------------------------------

So, unless we finally fix the CPOW rule to ignore ContentTask, we don't have any other options than to either disable the rule altogether, or disable lines of code as you did.
Fixing the rule should be easy. I'll take a look at it now.
Attachment #8740626 - Flags: feedback?(mratcliffe)
This makes the CPOW rule ignore ContenTask.spawn, and turns the rule to error so it's not just warning noise in the logs anymore.
Now, of course, this can't catch things like

  let location = yield spawnViewportTask(ui, {}, function* () {
    return content.location.href;
  });

where spawnViewportTask actually wraps ContentTask.spawn. Fixing this would be a whole lot more difficult. So, I suggest doing this for these cases:

  let location = yield spawnViewportTask(ui, {}, function* () {
    return content.location.href; // eslint-disable-line
  });

which is essentially what you did in the first patch Alex, but hopefully this should be reduced to only a few places instead of everywhere we use ContentTask.
Attachment #8740626 - Attachment is obsolete: true
Attachment #8740627 - Attachment is obsolete: true
Attachment #8740849 - Flags: review?(poirot.alex)
Sorry for stealing this bug :( but this should get rid of all warnings, so hopefully it's for the best :)

I'm wondering if we should not simply disable the CPOW rule after all ... we now have CPOW usage checks in tests, so I don't know that this rule is really all that helpful now.
Comment on attachment 8740848 [details] [diff] [review]
Bug_1264063_-_1_-_Disable_prop-types_eslint_rule_a.diff

Review of attachment 8740848 [details] [diff] [review]:
-----------------------------------------------------------------

A man of action!
I like when throwing patches ends up receiving even better ones ;)
Attachment #8740848 - Flags: review?(poirot.alex) → review+
Blocks: 1251271
Comment on attachment 8740849 [details] [diff] [review]
Bug_1264063_-_2_-_Make_the_CPOW_rule_log_errors_an.diff

Review of attachment 8740849 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Patrick Brosset [:pbro] from comment #11)
> Sorry for stealing this bug :( but this should get rid of all warnings, so
> hopefully it's for the best :)
> 
> I'm wondering if we should not simply disable the CPOW rule after all ... we
> now have CPOW usage checks in tests, so I don't know that this rule is
> really all that helpful now.

Does this check works in mochitest browser? Does it make the test fail and reject landing?
It does work in optimized build, i.e. there is no need to be un debug to have this check?
If the answer is yes to all these questions. Then yes, let just make eslint faster.
If you modify a test, you will at least run it locally and get the runtime exception,
so eslint doesn't save you from running something here.

::: testing/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js
@@ +45,5 @@
> +  }
> +
> +  function isInContentTask(node) {
> +    while (node.parent) {
> +      var expression = node.argument ? node.argument.callee : node;

I'm not a eslint expert so I don't really follow this.
But the while() loop seems possibly expensive if there is many parent.
Attachment #8740849 - Flags: review?(poirot.alex)
Keywords: leave-open
Comment on attachment 8740849 [details] [diff] [review]
Bug_1264063_-_2_-_Make_the_CPOW_rule_log_errors_an.diff

Took another approach to fix the no-cpow rule to avoid the while loop. Simply by adding AST listeners at the right moments we can track when we enter and exit a ContentTask.spawn execution and therefore ignore CPOW warnings then.
Attachment #8740849 - Attachment is obsolete: true
New pending try build (ESLint job successful, which is what matters here): https://treeherder.mozilla.org/#/jobs?repo=try&revision=61df5e57d4f3
Comment on attachment 8741103 [details]
MozReview Request: Bug 1264063 - 2 - Make the CPOW rule log errors and ignore ContentTask.spawn; r=Mossop

https://reviewboard.mozilla.org/r/46201/#review42727
Attachment #8741103 - Flags: review?(dtownsend) → review+
Keywords: leave-open
Assignee: poirot.alex → pbrosset
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/bba6594effdc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
reopen as a 2nd patch need landing
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/c61fcac04879
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1245779
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.