Closed Bug 1375903 Opened 7 years ago Closed 7 years ago

Enable eslint on testing/talos

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

Attachments

(2 files, 1 obsolete file)

enable eslint on testing/talos - mechanical changes
139.14 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
additional changes
56.43 KB, patch
jmaher
: review+
Details | Diff | Splinter Review 
      No description provided.
This is a first attempt using 'mach eslint --fix' to address most issues; about 1000 issues remain. :(

Would appreciate your feedback: Are there additional files that could/should be ignored?
Attachment #8880865 - Flags: feedback?(jmaher)
Comment on attachment 8880865 [details] [diff] [review]
inital attempt with |mach eslint --fix|

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

we should ignore:
testing/talos/talos/tests/canvasmark/**
testing/talos/talos/tests/dromaeo/**
testing/talos/talos/tests/v8_7/**
testing/talos/talos/tests/kraken/**

please add these exceptions to tools/rewriting/ThirdPartyPaths.txt also :)

I don't mind editing them, but save trouble and lets ignore those external benchmarks.

::: testing/talos/talos/tests/webgl/benchmarks/terrain/perftest.html
@@ +1,1 @@
> +<html>

huh? this is a lot of new code, I am unclear why this is in the patch?
Attachment #8880865 - Flags: feedback?(jmaher) → feedback+
(In reply to Joel Maher ( :jmaher) from comment #2)
> huh? this is a lot of new code, I am unclear why this is in the patch?

There is a lot of inconsistency in CR/LF use in that file...it seems to have confused 'eslint --fix'. I'll backout changes to that file and hand edit instead.

Thanks!
Mechanical changes, mostly made with 'mach eslint --fix'.
Attachment #8880865 - Attachment is obsolete: true
Attachment #8881505 - Flags: review?(jmaher)
Additional changes, generally made by hand.

eslint found unused code, missing returns, and a variety of other issues.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7625d4d73861c473c86bf712780483dbb07594a4
Attachment #8881510 - Flags: review?(jmaher)
Comment on attachment 8881505 [details] [diff] [review]
enable eslint on testing/talos - mechanical changes

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

a few nits

::: testing/talos/talos/talos-powers/components/TalosPowersService.js
@@ +302,4 @@
>        let mm = msg.target.messageManager;
>        mm.sendAsyncMessage("TalosPowers:ParentExec:ReplyMsg", {
>          id: msg.data.id,
> +        result

why is id: still specified, but we removed result: ?

::: testing/talos/talos/tests/tabswitch/bootstrap.js
@@ +23,5 @@
>  
>      // Wait for the window to finish loading
>      let window = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);
>      let cb = function() {
> +      window.removeEventListener("load", cb);

'false' is removed?

@@ +28,3 @@
>        loadIntoWindow(window);
>      };
> +    window.addEventListener("load", cb);

'false' is removed?

@@ +67,4 @@
>          Services.obs.removeObserver(onStartup, topic);
>          resolve();
>        }
> +    }, topic);

why is 'false' removed?

@@ +554,4 @@
>    // Load into any new windows
>    Services.wm.addListener(windowListener);
>  
> +  Services.obs.addObserver(observer, "tabswitch-urlfile");

'false' is removed?

::: testing/talos/talos/tests/tart/addon/content/tart.html
@@ +23,5 @@
>      new CustomEvent("tart@mozilla.org:chrome-exec-event", {
>        bubbles: true,
>        detail: {
>          command: {
>            name: commandName,

why is 'name' still in here?
Attachment #8881505 - Flags: review?(jmaher) → review+
Comment on attachment 8881510 [details] [diff] [review]
additional changes

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

lots of great changes here.

::: testing/talos/talos/tests/webgl/benchmarks/terrain/perftest.html
@@ -1,1 @@
> -<html>

are all these changes newlines?
Attachment #8881510 - Flags: review?(jmaher) → review+
(In reply to Joel Maher ( :jmaher) from comment #7)
> Comment on attachment 8881510 [details] [diff] [review]
> additional changes
> 
> Review of attachment 8881510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lots of great changes here.
> 
> ::: testing/talos/talos/tests/webgl/benchmarks/terrain/perftest.html
> @@ -1,1 @@
> > -<html>
> 
> are all these changes newlines?

Almost all of the changes are newlines. There are also changes in reportResults():
 - else block eliminated because it follows a return
 - return added in the "else" case
(In reply to Joel Maher ( :jmaher) from comment #6)
> Comment on attachment 8881505 [details] [diff] [review]
> enable eslint on testing/talos - mechanical changes
> 
> Review of attachment 8881505 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> a few nits

Generally, "why" these changes are made (and why others are not) can only be answered "because that's what our eslint rules say". I'm late to the game and largely removed from the evolution of those rules, so I sometimes don't have a lot of insight.


> ::: testing/talos/talos/talos-powers/components/TalosPowersService.js
> @@ +302,4 @@
> >        let mm = msg.target.messageManager;
> >        mm.sendAsyncMessage("TalosPowers:ParentExec:ReplyMsg", {
> >          id: msg.data.id,
> > +        result
> 
> why is id: still specified, but we removed result: ?

In option parameters, "x: x" is redundant and eslint wants that reduced to simply "x"; I believe no such simplification is possible for "x: y".

> ::: testing/talos/talos/tests/tabswitch/bootstrap.js
> @@ +23,5 @@
> >  
> >      // Wait for the window to finish loading
> >      let window = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);
> >      let cb = function() {
> > +      window.removeEventListener("load", cb);
> 
> 'false' is removed?

Yes -- it is an optional parameter with false as the default, so no change in behavior, and simpler.

> @@ +28,3 @@
> >        loadIntoWindow(window);
> >      };
> > +    window.addEventListener("load", cb);
> 
> 'false' is removed?

Ditto.

> @@ +67,4 @@
> >          Services.obs.removeObserver(onStartup, topic);
> >          resolve();
> >        }
> > +    }, topic);
> 
> why is 'false' removed?

As above.

> @@ +554,4 @@
> >    // Load into any new windows
> >    Services.wm.addListener(windowListener);
> >  
> > +  Services.obs.addObserver(observer, "tabswitch-urlfile");
> 
> 'false' is removed?

As above.

> ::: testing/talos/talos/tests/tart/addon/content/tart.html
> @@ +23,5 @@
> >      new CustomEvent("tart@mozilla.org:chrome-exec-event", {
> >        bubbles: true,
> >        detail: {
> >          command: {
> >            name: commandName,
> 
> why is 'name' still in here?

The "x: y" case, above.


https://gecko.readthedocs.io/en/latest/tools/lint/linters/eslint-plugin-mozilla.html has more info on Mozilla eslint rules like "no-useless-parameters". http://eslint.org/docs/rules/ has more info on standard rules like "object-shorthand".
thanks :gbrown, I would be happier with manually making |x: y| -> |y| to match the changes made.  Everything else looks good.
(In reply to Joel Maher ( :jmaher) from comment #10)
> thanks :gbrown, I would be happier with manually making |x: y| -> |y| to
> match the changes made.  Everything else looks good.

So you can't do that in the case where y is the property of a different object (Javascript doesn't support this). You can only do that for y being a variable name.
got it- this is what I get for writing more python than js code.
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5100785d8f
Enable eslint on testing/talos - mechanical changes; r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/a13cfd0bbb4f
Enable eslint on testing/talos - additional changes; r=jmaher
https://hg.mozilla.org/mozilla-central/rev/9a5100785d8f
https://hg.mozilla.org/mozilla-central/rev/a13cfd0bbb4f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Looks like these changes brought some noticeable improvements:

== Change summary for alert #7577 (as of June 28 2017 12:58 UTC) ==

Improvements:

 37%  glterrain summary windows10-64 opt e10s     4.67 -> 2.96
 37%  glterrain summary windows10-64 pgo e10s     4.70 -> 2.98
 33%  glterrain summary windows7-32 pgo e10s      5.09 -> 3.42
 32%  glterrain summary windows7-32 opt e10s      5.08 -> 3.46
 20%  glterrain summary linux64 opt e10s          10.65 -> 8.54
 19%  glterrain summary linux64 pgo e10s          10.49 -> 8.51
 14%  glterrain summary osx-10-10 opt e10s        4.04 -> 3.47

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7577
:gbrown Were you expecting improvements out of this bug? Please confirm, because your changes seem to involve only the testing code.
Flags: needinfo?(gbrown)
Seeing as the patch touched perftest.html (mainly changed unix line endings, about 40-60 bytes smaller), it is conceivable that it affected the test timings.
I was half-expecting some change to Talos results, simply due to the scope of the changes (even though they are trivial) -- but nothing specific. I agree with Mark's comment and can't think of a better explanation.

I would suggest simply accepting the improvement, but if there is any objection, let me know and I'll try backing out the perftest.html change, or investigate further.
Flags: needinfo?(gbrown)
Product: Testing → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: