Enable disabled eslint rules and fix corresponding js problems

RESOLVED FIXED

Status

Tree Management
Treeherder
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: wlach, Assigned: ShrutiJ)

Tracking

(Blocks: 1 bug)

Details

Attachments

(2 attachments)

Working on bug 1289127, I realized there's a bunch of rules we ought to enable for eslint, but are currently disabled:

https://github.com/mozilla/treeherder/blob/master/.eslintrc#L10

(you can just flip them from "0" to "2", the run "grunt checkjs" to find what fails, fix, and repeat)

Shruti, do you want to try your hand at fixing these? Might be a good fast JavaScript learning experience between working on other bugs. :)
Blocks: 1183749
Created attachment 8775028 [details] [review]
[treeherder] SJasoria:Bug_1289138 > mozilla:master
(Assignee)

Comment 2

2 years ago
Comment on attachment 8775028 [details] [review]
[treeherder] SJasoria:Bug_1289138 > mozilla:master

I am not able to fix 2 no-undef errors:

https://github.com/mozilla/treeherder/blob/master/ui/js/filters.js#L20
Here platformNameMap throws no-undef, and

https://github.com/mozilla/treeherder/blob/master/ui/js/perf.js#L92
here jobId throws no-undef

How should I go about fixing these to errors? 

Other then these 2, I have tried to fix all of the errors. Please see and let me know if any change has to be made.
Attachment #8775028 - Flags: feedback?(wlachance)
Comment on attachment 8775028 [details] [review]
[treeherder] SJasoria:Bug_1289138 > mozilla:master

A bunch of things don't seem quite right and this needs to be rebased against master, but looking good so far!
Attachment #8775028 - Flags: feedback?(wlachance) → feedback+
(Assignee)

Comment 4

2 years ago
Comment on attachment 8775028 [details] [review]
[treeherder] SJasoria:Bug_1289138 > mozilla:master

One more doubt:

In https://github.com/SJasoria/treeherder/blob/f46edcfcef051f9c788cb75790e90f23d8dced1e/ui/js/controllers/jobs.js#L224

getBuildbotRequestId is giving no-undef error on running grunt checkjs.

I found this function here:
https://github.com/mozilla/treeherder/blob/master/ui/plugins/controller.js#L332

Is it a good idea to add 'ThJobArtifactModel' to jobs.js and then pull the data to get request_id the way it is done in controller.js?

Other then this, I have addressed all the comments you made in PR :)
Attachment #8775028 - Flags: feedback+ → feedback?
(In reply to Shruti Jasoria [:ShrutiJ] from comment #4)
> Comment on attachment 8775028 [details] [review]
> [treeherder] SJasoria:Bug_1289138 > mozilla:master
> 
> One more doubt:
> 
> In
> https://github.com/SJasoria/treeherder/blob/
> f46edcfcef051f9c788cb75790e90f23d8dced1e/ui/js/controllers/jobs.js#L224
> 
> getBuildbotRequestId is giving no-undef error on running grunt checkjs.
> 
> I found this function here:
> https://github.com/mozilla/treeherder/blob/master/ui/plugins/controller.
> js#L332
> 
> Is it a good idea to add 'ThJobArtifactModel' to jobs.js and then pull the
> data to get request_id the way it is done in controller.js?

Yes, you've found a bug! This code is currently broken. I believe your solution is correct too: we need to get the buildbot request id to send a manual cancellation request. This code is a bit tricky to test properly (you need credentials to do it), so I'll just do that myself tomorrow.
Depends on: 1290525
Shruti: this looks great to me.  Just two small fixes in comments on the PR.  Please let me know when you're done.  Since a lot of this code will be affected by minimization, I'd like to test deploying to stage before we merge it.  So I'll do that and then merge if all looks good.

Thanks again!
(Assignee)

Comment 7

2 years ago
Comment on attachment 8775028 [details] [review]
[treeherder] SJasoria:Bug_1289138 > mozilla:master

I have made the changes which you had asked for in the PR. I hope its god now.
Attachment #8775028 - Flags: review?(cdawson)
(Assignee)

Updated

2 years ago
Attachment #8775028 - Flags: feedback?
This looks good to me.  Thanks for doing this!  I'm going to try pushing this to stage to test it.
Comment on attachment 8775028 [details] [review]
[treeherder] SJasoria:Bug_1289138 > mozilla:master

Great work!
Attachment #8775028 - Flags: review?(cdawson) → review+

Comment 10

2 years ago
Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/4c8d3f69e67e5608c6241fe0bca69a78af150fe6
Bug 1289138 - Enable disabled eslint rules and fix corresponding js problems (#1736)

Enable the following rules and fixed corresponding problems:
* comma-dangle
* no-console
* no-redeclare
* no-unused-vars
* no-undef
* no-unused-vars
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Created attachment 8780703 [details] [review]
[treeherder] wlach:1289138-2 > mozilla:master

Comment 12

2 years ago
Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/9b2bcda2c1da968255c599f32688e7ed7f10341d
Bug 1289138 - Remove explicit enabling of rules that are enabled by default (#1785)

Now that these rules are working, we don't need to explicitly enable them.
You need to log in before you can comment on or make changes to this bug.