Eslint cleanup of memory tool code

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Memory
P2
normal
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: ntim, Assigned: Thomas Dräbing, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

unspecified
Firefox 53
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

a year ago
Mentor: ntim.bugs@gmail.com
Whiteboard: [good first bug][lang=js]

Comment 1

a year ago
Hello,

I would like to take this bug as my University of Toronto Assignment for a good first bug. Please advise as to where to look for memory tool code.
(Reporter)

Comment 2

a year ago
(In reply to Cody from comment #1)
> Hello,
> 
> I would like to take this bug as my University of Toronto Assignment for a
> good first bug. Please advise as to where to look for memory tool code.

Thanks for your interest!

First of all, you need to set up your development environment, the fastest way to do it is to follow the instructions here: https://hacks.mozilla.org/2016/02/introducing-devtools-reload/

Once you've done so:
- Setup eslint by running `./mach eslint --setup`
- Check all the eslint errors in the memory tool code by running: `./mach eslint devtools/client/memory/ --no-ignore`
- Fix those errors by editing the JS files in the `devtools/client/memory`
- Run eslint again to make sure you haven't forgotten any error or added any
- Run the Memory tool for a test drive (you need to enable it from the options panel inside the developer tools), if everything works fine, generate a diff and post it here. The blog post I linked above should give enough information on how to test your own copy of developer tools.

If you need any help, please use the need more information box located after the comment box. You can also ask other people around the #devtools or #introduction channel on IRC.
(Reporter)

Updated

a year ago
Assignee: nobody → codyhowarth

Comment 3

a year ago
Hi Tim,

Thanks for the setup instructions.

I've been able to run eslint and check out some of the errors. Most them I understand and should be able to fix no problem. However, there are a lot of unused-vars errors, and while it seems like those variables aren't used in the particular file they are in error for, I'm assuming they may be needed elsewhere? If they should actually be removed that should be no problem.
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 4

a year ago
(In reply to Cody from comment #3)
> Hi Tim,
> 
> Thanks for the setup instructions.
> 
> I've been able to run eslint and check out some of the errors. Most them I
> understand and should be able to fix no problem. However, there are a lot of
> unused-vars errors, and while it seems like those variables aren't used in
> the particular file they are in error for, I'm assuming they may be needed
> elsewhere? If they should actually be removed that should be no problem.

If they aren't used elsewhere, then they should be removed. A simple search on http://dxr.mozilla.org (with your queries prefixed with "path:devtools/client/memory ") should tell you if they are used elsewhere or not. 
If they are, in that case, add `/* exported variable1, variable2 */` at the beginning of the file.
Flags: needinfo?(ntim.bugs)
Has STR: --- → yes
Priority: -- → P2

Comment 5

a year ago
Created attachment 8725455 [details] [diff] [review]
Patch for Bug1251728

I've generated a patch. However there is still 1 error I'm not sure how to fix:

 0:00.08 Running /usr/bin/eslint
 0:00.08 /usr/bin/eslint --plugin html --ext [.js,.jsm,.jsx,.xml,.html] devtools/client/memory/ --no-ignore

/gecko-dev/devtools/client/memory/test/browser/browser_memory_breakdowns_01.js
  30:40  error  "el" is already declared in the upper scope  no-shadow

✖ 1 problem (1 error, 0 warnings)

It seems that the variable is declared partially in terms of itself. 

Please advise how to move forward.

 0:02.97 Finished eslint. Errors encountered.

Comment 6

a year ago
(In reply to Cody from comment #5)

Oops, the last line of the previous comment was supposed to be under "1 problem".
Flags: needinfo?(ntim.bugs)
(Reporter)

Updated

a year ago
Attachment #8725455 - Flags: feedback+
(Reporter)

Comment 7

a year ago
Comment on attachment 8725455 [details] [diff] [review]
Patch for Bug1251728

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

::: devtools/client/memory/test/browser/browser_memory_breakdowns_01.js
@@ +26,5 @@
>    [L10N.getStr("tree-item.nostack")].forEach(findNameCell);
>  
> +  function findNameCell(name) {
> +    let el = Array.prototype.find.call($$(".tree .heap-tree-item-name span"),
> +                                       el => el.textContent === name);

What you'd want to do here is use a different variable name for `el` in `el => el.textContent === name`. It should fix the issue without changing how things work.
(Reporter)

Updated

a year ago
Flags: needinfo?(ntim.bugs)

Comment 8

a year ago
Created attachment 8725490 [details] [diff] [review]
Patch for Bug1251728 with variable shadow fixed.

Ok thanks! Here's the patch with that issue fixed and currently showing no eslint errors.
Attachment #8725455 - Attachment is obsolete: true
(Reporter)

Comment 9

a year ago
Comment on attachment 8725490 [details] [diff] [review]
Patch for Bug1251728 with variable shadow fixed.

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

Can you fold this with your previous patch? This patch only contains the browser_memory_breakdowns_01.js change

::: devtools/client/memory/test/browser/browser_memory_breakdowns_01.js
@@ +31,1 @@
>      ok(el, `Found heap tree item cell for ${name}.`);

Maybe the outer variable should be called cell, while the inner variable should be named el.
Like this:     
  let cell = Array.prototype.find.call($$(".tree .heap-tree-item-name span"), 
                                       el => el.textContent === name);
  ok(cell, `Found heap tree item cell for ${name}.`);
(Reporter)

Comment 10

a year ago
I'll be busy, so please ask Patrick Brosset for review, or if you need any information about eslint.

Comment 11

a year ago
Created attachment 8725763 [details] [diff] [review]
Proposed patch for Bug 1251728

Ok, thanks Tim. Here are the patches folded together, with the variable name in question changed to cell. I've requested review from Patrick.
Attachment #8725490 - Attachment is obsolete: true
Attachment #8725763 - Flags: review?(pbrosset)
Comment on attachment 8725763 [details] [diff] [review]
Proposed patch for Bug 1251728

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

Thanks for the cleanup.
I assume all eslint errors are gone with this change? If so, then you should also remove this line from .eslintignore:
devtools/client/memory/**
so that we start checking the whole directory during CI.

Also, minor comment: can you change the commit message to end with r=pbrosset instead of r=Patrick Brosset

Now, I have made some comments in the code changes below, but I have a few other remarks that apply to several files:

- about /*exported ... */ comments
I don't think this is the right solution, if a module exports something, then it most probably uses `exports.something = ...` already.
I see a lot of them do something like: `const something = exports.something = ...` and so eslint complains that `something` isn't used.
I would like to argue that we remove the `const something = ` part and just leave the `exports.something = ...` part.
Adding /*exported */ comments means that we have to maintain these comments manually when things change.

- there's a bunch of /*globals waitUntilState */ and other similar globals defined in tests.
There's a better way to import these globals for eslint to know about them.
First of all, you should know that globals from the corresponding head.js file are imported automatically. Then, if head.js loads some other files (like shared-head.js and shared-redux-head.js), then the globals from these files can be imported in eslint by using a special comment:
/* import-globals-from path/to/loaded/file.js */
So, in the case of /devtools/client/memory/test/browser/head.js
you can add these 2 comments:
/* import-globals-from ../../../framework/test/shared-head.js */
/* import-globals-from ../../../framework/test/shared-redux-head.js */
This way, eslint will know about the globals in these files, import them in head.js, and in turn, all tests in the same directory as this head.js will get them too. So no need for any /*globals */ comment in those tests.

- For head.js files: these files typically contain a lot of globals that are only used in other files. I see you've used a bunch of /*exported */ comments there too.
So far, we've been doing this instead:
/* eslint no-unused-vars: [2, {"vars": "local", "args": "none"}] */
This tells eslint not to worry about global variables that aren't used inside head.js files.
I think this is less maintenance burden than listing all variables that are being exported.

::: devtools/client/memory/test/unit/test_dominator_trees_04.js
@@ +29,5 @@
>    for (let intermediateSnapshotState of [states.SAVING,
>                                           states.READING,
>                                           states.SAVING_CENSUS]) {
> +    dumpn(`Testing switching to the DOMINATOR_TREE view
> +          in the middle of the ${intermediateSnapshotState} snapshot state`);

The problem with wrapping in a template string is that whitespaces are going to appear in the output now:

Testing switching to the DOMINATOR_TREE view
         in the middle of the ... snapshot state

::: devtools/client/memory/utils.js
@@ +99,4 @@
>    } catch (e) {
>      DevToolsUtils.reportException(
> +      `String stored in "${CUSTOM_BREAKDOWN_PREF}"
> +       pref cannot be parsed by \`JSON.parse()\`.`);

Wrapping in a template string means that the string will wrap on 2 lines in the browser console too. Might not be a problem, but if you don't want this to happen, you should use a quoted string instead.

@@ +122,1 @@
>    // If it's in our custom breakdowns, use it

I believe It makes more sense to have the comment inside the if block instead of just above.
Attachment #8725763 - Flags: review?(pbrosset)
(Reporter)

Updated

9 months ago
Assignee: codyhowarth → nobody
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug

Comment 14

7 months ago
Hi Tim,

I'm interested in this bug.

I've installed ESLint and its plugins with './mach eslint --setup', but when trying to check the ESLint errors in the memory tool code, I got this:

    mozilla-central xfq$ ./mach eslint devtools/client/memory/ --no-ignore
    ✖ 0 problems (0 errors, 0 warnings)

Any insight?
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 15

7 months ago
(In reply to Xue Fuqiao [:xfq] from comment #14)
> Hi Tim,
> 
> I'm interested in this bug.
> 
> I've installed ESLint and its plugins with './mach eslint --setup', but when
> trying to check the ESLint errors in the memory tool code, I got this:
> 
>     mozilla-central xfq$ ./mach eslint devtools/client/memory/ --no-ignore
>     ✖ 0 problems (0 errors, 0 warnings)
> 
> Any insight?
Simply running: eslint devtools/client/memory/ --no-ignore (without the ./mach) works for me, and displays the errors.

Also note that you can use eslint devtools/client/memory/ --fix to fix some errors (although it won't fix all of them).
Flags: needinfo?(ntim.bugs)

Comment 16

7 months ago
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #15)
> (In reply to Xue Fuqiao [:xfq] from comment #14)
> > I've installed ESLint and its plugins with './mach eslint --setup', but when
> > trying to check the ESLint errors in the memory tool code, I got this:
> > 
> >     mozilla-central xfq$ ./mach eslint devtools/client/memory/ --no-ignore
> >     ✖ 0 problems (0 errors, 0 warnings)
> > 
> > Any insight?
> Simply running: eslint devtools/client/memory/ --no-ignore (without the
> ./mach) works for me, and displays the errors.
> 
> Also note that you can use eslint devtools/client/memory/ --fix to fix some
> errors (although it won't fix all of them).

$ eslint devtools/client/memory/ --no-ignore
-bash: eslint: command not found

Then I tried this:

$ tools/lint/eslint/node_modules/.bin/eslint devtools/client/memory/ --no-ignore

and finally succeeded with '✖ 401 problems (400 errors, 1 warning)'.

I'll try to fix them later. Thank you!
(Assignee)

Comment 17

4 months ago
Created attachment 8820607 [details] [diff] [review]
Linted code files following ESLint rules
(Assignee)

Comment 18

4 months ago
Hi,

I created a patch, in which I linted the respective files. I followed the hints given in earlier comments. ESLint is now giving 0 errors. Please review my patch. If anything should be done differently, I am happy to apply further changes to the patch.

Thanks and best,
Thomas
(Reporter)

Updated

4 months ago
Attachment #8820607 - Flags: review?(ntim.bugs)
(Reporter)

Updated

4 months ago
Attachment #8725763 - Attachment is obsolete: true
(Reporter)

Comment 19

4 months ago
Comment on attachment 8820607 [details] [diff] [review]
Linted code files following ESLint rules

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

Looks great overall! Thanks for taking your time to go through these files.

My main concern is putting /* exported run_test */ on every unit test.

Creating an .eslintrc.js file should spare you from those comments.

::: devtools/client/memory/actions/census-display.js
@@ +30,1 @@
>  

You can write:
    "Breakdowns must be an object with a `by` property, attempted to set: " +
    uneval(display)

::: devtools/client/memory/actions/diffing.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* exported snapshotActions */
> +

Are you sure snapshotActions is used elsewhere ?

::: devtools/client/memory/actions/io.js
@@ +68,5 @@
>    };
>  };
>  
> +const importSnapshotAndCensus =
> +  exports.importSnapshotAndCensus = function (heapWorker, path) {

I'd rather do:
const importSnapshotAndCensus = function(...) {
 ...
};
exports.importSnapshotAndCensus = importSnapshotAndCensus;

since it spares us from indenting the whole block

::: devtools/client/memory/actions/label-display.js
@@ +28,5 @@
>    assert(typeof display === "object"
>           && display
>           && display.breakdown
>           && display.breakdown.by,
> +    `Breakdowns must be an object with a \`by\` property, attempted to set: ` +

Same comment as previously.

::: devtools/client/memory/actions/tree-map-display.js
@@ +28,5 @@
>           && display
>           && display.breakdown
>           && display.breakdown.by,
> +    `Breakdowns must be an object with a \`by\` property, attempted to set: ` +
> +    `${uneval(display)}`);

Same comment as previously

::: devtools/client/memory/initializer.js
@@ +53,5 @@
> +  gRoot = null;
> +  gApp = null;
> +  gProvider = null;
> +  unsubscribe = null;
> +  isHighlighted = null;

nit: 
use 
`gStore = gRoot = gApp = gProvider = unsubscribe = isHighlighted = null;`

::: devtools/client/memory/reducers/snapshots.js
@@ +273,1 @@
>  

This is the same as:
        "Should have already computed the dominator tree, found state = " +
        snapshot.dominatorTree.state

::: devtools/client/memory/test/browser/head.js
@@ +5,5 @@
> +/* import-globals-from ../../../framework/test/shared-redux-head.js */
> +
> +/* eslint no-unused-vars: [2, {"vars": "local", "args": "none"}] */
> +
> +/* global censusState */

You should be able to add as eslintrc file like this one: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsive.html/test/browser/.eslintrc.js

And a lot of var-related errors should disappear.

::: devtools/client/memory/test/unit/test_action-clear-snapshots_01.js
@@ +1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +/* exported run_test */

The eslintrc file should spare you from doing that.

::: devtools/client/memory/test/unit/test_dominator_trees_04.js
@@ +30,5 @@
>    yield front.attach();
>  
> +  for (let intermediateSnapshotState of [states.SAVING, states.READING, states.READ]) {
> +    dumpn(`Testing switching to the DOMINATOR_TREE view in the middle of the ` +
> +          `${intermediateSnapshotState} snapshot state`);

The first line can be a normal string now.
Attachment #8820607 - Flags: review?(ntim.bugs) → review+
(Assignee)

Comment 20

4 months ago
Created attachment 8820720 [details] [diff] [review]
Linted code files following ESLint rules. Changes now include comments of Tim Nguyen
(Assignee)

Updated

4 months ago
Attachment #8820607 - Attachment is obsolete: true
(Assignee)

Comment 21

4 months ago
Hi Tim,

thanks for the helpful comments. I made the corresponding changes and attached a patch containing the combined changes.

Best,
Thomas
(Reporter)

Comment 22

4 months ago
Created attachment 8820776 [details] [diff] [review]
memory-tool-eslint

Hey Thomas,
Thanks for your new patch. It had a few issues, so I've fixed them in this patch (I've kept your commit info though). It is now ready for landing.

I've pushed your patch to the try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27267aefc28f7f74ba668480b728914307737e5c
Attachment #8820776 - Flags: review+
(Reporter)

Comment 23

4 months ago
Just a reminder to check the try results and land if everything looks alright.
Flags: needinfo?(ntim.bugs)

Comment 24

4 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ad068aeaaf
Make devtools/client/memory/ eslint clean. r=ntim
(Reporter)

Comment 25

4 months ago
(In reply to Pulsebot from comment #24)
> Pushed by ntim.bugs@gmail.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ad068aeaaf
> Make devtools/client/memory/ eslint clean. r=ntim

Thomas, I've just landed your patch on mozilla-inbound, it should be in Nightly in two days. Congrats on your first patch!
Assignee: nobody → thomas.draebing
Status: NEW → ASSIGNED
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 26

4 months ago
Thank you so much Tim for your reviews and support. It was a fun task to start out with. I will certainly look out for other opportunities to contribute.

Comment 27

4 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8303c228d2d9
Followup: fix memory tool Xpcshell bustage due to introduction of strict mode. r=bustage

Comment 28

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f9ad068aeaaf
https://hg.mozilla.org/mozilla-central/rev/8303c228d2d9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.