1326331 - Fix Eslint errors in devtools/shared/heapsnapshot/

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Tim Nguyen :ntim]

Run `./mach eslint devtools/shared/heapsnapshot/ --no-ignore` to see the errors.

Comment 1

[Ruturaj Vartak]

Before going thru any changes..  I ran the following

=================================================
$ ./mach test devtools/shared/heapsnapshot/

.. it stalls on a firefox window (http://mochi.test:8888/tests?autorun=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp)

I had to manually close this window, post which ...

Summary
=======

Ran 83 tests
Expected results: 81
Unexpected results: 0
Skipped: 2

===========================================

$ ./mach mochitest devtools/shared/heapsnapshot/ --disable-e10s

Again it stalls on same firefox window (http://mochi.test:8888/tests?autorun=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp)

after closing it..

...
Buffered messages finished
0 INFO TEST-START | Shutdown
1 INFO Passed:  1
2 INFO Failed:  0
3 INFO Todo:    0
4 INFO Mode:    non-e10s
5 INFO SimpleTest FINISHED
Buffered messages finished
SUITE-END | took 10s
==============================


Just wanted to check if that is the right way of testing ?

Comment 2

[Ruturaj Vartak]

Tim, could you check comment 1?

Comment 3

[Tim Nguyen :ntim]

Seems fine to me! The two commands should cover the unit and mochitest test directories. 

There's also a gtest directory that contains C++ tests, although I don't really know how to run those tests. Although I don't think changes in JS files should affect the C++ tests. Maybe :sfink can shed a light?

Comment 4

[J. Ryan Stinnett [:jryans]]

(In reply to Ruturaj Vartak from comment #1)
> =================================================
> $ ./mach test devtools/shared/heapsnapshot/
> I had to manually close this window, post which ...

Hmm, I think it's okay...?  There are some test types that leave a window open, but I thought that only happened when running a single test file, not a directory...

Ah, just tried it locally.  One of the directories it runs is `devtools/shared/heapsnapshot/tests/mochitest`, which only contains one test, so this triggers the "stay open for one test" behavior.  So, it's a bit strange, but I guess that's expected behavior.

Also, you have to look at the results kind of carefully, because it actually runs several suites and directories, each of which can print separate result summaries.  You can try searching the log output for "INFO Passed:" to make sure you see all the summaries.

Overall, sounds correct and it's just our test harness being a bit strange.

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to Tim Nguyen :ntim from comment #3)
> Seems fine to me! The two commands should cover the unit and mochitest test
> directories. 
> 
> There's also a gtest directory that contains C++ tests, although I don't
> really know how to run those tests. Although I don't think changes in JS
> files should affect the C++ tests. Maybe :sfink can shed a light?

I'm not the one to ask about gtests, and I'm not sure who is. That said, my limited understanding is that gtests are exactly what you think, and will only test the C++ code. (In theory, you could write a gtest that evaluated JS code while it was running, but you'd probably put that near the JS engine or something.)

At any rate, I do not know of any reason why running the gtests would be necessary.

Comment 6

[Ruturaj Vartak]

Attachment: wip-fix-1326331.patch

WIP

--------------------------------------

Not sure what is the ref. of `this` here - https://dxr.mozilla.org/mozilla-central/source/devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_takeCensus_05.js#16
If I put "use strict"; , `this` becomes undefined. 

Same problem undefined globals in test_SaveHeapSnapshot.js

- Not sure how to fix
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_takeCensus_07.js#15
It gives  no-throw-literal error, I tried throw Error("ಠ_ಠ"), throw new String("ಠ_ಠ")
all fail tests, so i've disabled nextline for now..


========================================================
Following is the existing eslint o/p - its related to the above scenario.

$ ./mach eslint  devtools/shared/heapsnapshot/  --no-ignore
/home/rutu/code/gecko-dev/devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_takeCensus_05.js
  9:1  error  Use the global form of 'use strict'.  strict (eslint)

/home/rutu/code/gecko-dev/devtools/shared/heapsnapshot/tests/unit/test_ReadHeapSnapshot.js
  6:1  error  Use the global form of 'use strict'.  strict (eslint)

/home/rutu/code/gecko-dev/devtools/shared/heapsnapshot/tests/unit/test_ReadHeapSnapshot_worker.js
  7:1  error  Use the global form of 'use strict'.  strict (eslint)

/home/rutu/code/gecko-dev/devtools/shared/heapsnapshot/tests/unit/test_SaveHeapSnapshot.js
  6:1  error  Use the global form of 'use strict'.  strict (eslint)

✖ 4 problems (4 errors, 0 warnings)

Comment 7

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #6)
> Created attachment 8843570 [details] [diff] [review]
> wip-fix-1326331.patch
> 
> WIP
> 
> --------------------------------------
> 
> Not sure what is the ref. of `this` here -
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/heapsnapshot/
> tests/unit/test_HeapSnapshot_takeCensus_05.js#16
> If I put "use strict"; , `this` becomes undefined. 
> 
> Same problem undefined globals in test_SaveHeapSnapshot.js
In strict mode, `this` is undefined for functions where it would normally refer to the global.

So `this` should correspond to the global, which is commonly `window`, but I'm not sure in this case. 

> - Not sure how to fix
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/heapsnapshot/
> tests/unit/test_HeapSnapshot_takeCensus_07.js#15
> It gives  no-throw-literal error, I tried throw Error("ಠ_ಠ"), throw new
> String("ಠ_ಠ")
> all fail tests, so i've disabled nextline for now..
These tests test for equality with "ಠ_ಠ". Since Error("ಠ_ಠ") and new String("ಠ_ಠ") are both considered objects in JS, they are compared by reference. 
So: Error("ಠ_ಠ") != Error("ಠ_ಠ"), same goes with new String("ಠ_ಠ"). They have to refer to the same thing in memory (variable) to be equal.

One solution is to change assertThrowsValue in head_heapsnapshot.js to check equality against the error message. The other would be working around the issue by doing:

get by() {
  let error = "ಠ_ಠ";
  throw error;
}


Ryan, can you confirm/correct these answers ?

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8843570 [details] [diff] [review]
wip-fix-1326331.patch

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

::: devtools/shared/heapsnapshot/CensusUtils.js
@@ +237,4 @@
>   * @overrides Visitor.prototype.enter
>   */
>  DiffVisitor.prototype.enter = function (breakdown, report, edge) {
> +  this._results === null;

You can remove this line.

@@ +377,4 @@
>   *
>   * @return {Object<number, TreeNode>}
>   */
> +const createParentMap = exports.createParentMap

I would prefer doing:

const createParentMap = function(node, getId = n => n.id, aggregator = {}) {
  ...
};
exports.createParentMap = createParentMap;

Comment 9

[Ruturaj Vartak]

(In reply to Tim Nguyen :ntim from comment #7)
> In strict mode, `this` is undefined for functions where it would normally
> refer to the global.
> So `this` should correspond to the global, which is commonly `window`, but
> I'm not sure in this case. 
Yes, I know ... I tried substituting `this` with `window` but I get `window undefined` error.

Comment 10

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #9)
> (In reply to Tim Nguyen :ntim from comment #7)
> > In strict mode, `this` is undefined for functions where it would normally
> > refer to the global.
> > So `this` should correspond to the global, which is commonly `window`, but
> > I'm not sure in this case. 
> Yes, I know ... I tried substituting `this` with `window` but I get `window
> undefined` error.

You could try:

var global = this;

function run_test() {
  ...
  global.ccw = g.allocationMarker();
}

Comment 11

[Tom Tromey :tromey]

(In reply to Tim Nguyen :ntim from comment #7)

> One solution is to change assertThrowsValue in head_heapsnapshot.js to check
> equality against the error message. The other would be working around the
> issue by doing:
> 
> get by() {
>   let error = "ಠ_ಠ";
>   throw error;
> }

It's better to throw an Error and change the test.
However if you do want to avoid the eslint error, it's better to use an
explicit suppression comment rather than obfuscate the code.
The reason I say this is that an explicit comment's intent is clear; but the above is not.

Comment 12

[J. Ryan Stinnett [:jryans]]

Comment on attachment 8843570 [details] [diff] [review]
wip-fix-1326331.patch

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

Looks okay overall, but please add the `.eslintrc` file as mentioned and take a look at the issues others have pointed out.

::: devtools/shared/heapsnapshot/tests/mochitest/test_saveHeapSnapshot_e10s_01.html
@@ +12,5 @@
>  </head>
>  <body>
>      <script type="application/javascript">
> +    "use strict";
> +    /* global window, ok, SimpleTest, ChromeUtils, sendAsyncMessage, info,

This directory needs a `.eslintrc` file so we can pull in the expected test globals.

Unfortunately, this directory mixes two test types together, but hopefully we'll still get something close to the expected result.  Add a `.eslintrc` similar to this one[1] (with the right number of ../ to reach /devtools).

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/shim/test/.eslintrc.js#5

Comment 13

[J. Ryan Stinnett [:jryans]]

(In reply to Tim Nguyen :ntim from comment #7)
> (In reply to Ruturaj Vartak from comment #6)
> > Created attachment 8843570 [details] [diff] [review]
> > wip-fix-1326331.patch
> > 
> > WIP
> > 
> > --------------------------------------
> > 
> > Not sure what is the ref. of `this` here -
> > https://dxr.mozilla.org/mozilla-central/source/devtools/shared/heapsnapshot/
> > tests/unit/test_HeapSnapshot_takeCensus_05.js#16
> > If I put "use strict"; , `this` becomes undefined. 
> > 
> > Same problem undefined globals in test_SaveHeapSnapshot.js
> In strict mode, `this` is undefined for functions where it would normally
> refer to the global.
> 
> So `this` should correspond to the global, which is commonly `window`, but
> I'm not sure in this case. 

It seems like the way this data is stored is meaningful for the purpose of the test, so I'd say suppress the rule in this case.

> > - Not sure how to fix
> > https://dxr.mozilla.org/mozilla-central/source/devtools/shared/heapsnapshot/
> > tests/unit/test_HeapSnapshot_takeCensus_07.js#15
> > It gives  no-throw-literal error, I tried throw Error("ಠ_ಠ"), throw new
> > String("ಠ_ಠ")
> > all fail tests, so i've disabled nextline for now..
> These tests test for equality with "ಠ_ಠ". Since Error("ಠ_ಠ") and new
> String("ಠ_ಠ") are both considered objects in JS, they are compared by
> reference. 
> So: Error("ಠ_ಠ") != Error("ಠ_ಠ"), same goes with new String("ಠ_ಠ"). They
> have to refer to the same thing in memory (variable) to be equal.
> 
> One solution is to change assertThrowsValue in head_heapsnapshot.js to check
> equality against the error message. The other would be working around the
> issue by doing:
> 
> get by() {
>   let error = "ಠ_ಠ";
>   throw error;
> }

I agree with :tromey that we should suppress the linting for this, since that's a bit more explicit.

Comment 14

[Ruturaj Vartak]

Attachment: fix-1326331-1.patch

- removed devtools/shared/heapsnapshot/** line from .eslintignore
- created .eslintrc.js in devtools/shared/heapsnapshot/tests/mochitest/ as suggested by :jryans as in comment#12
- I've reworked test_HeapSnapshot_takeCensus_07 changed the way the value is evaluated as originally suggested by :tromey ; comment#11

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8844377 [details] [diff] [review]
fix-1326331-1.patch

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

Redirecting review to Ryan

Comment 16

[J. Ryan Stinnett [:jryans]]

Comment on attachment 8844377 [details] [diff] [review]
fix-1326331-1.patch

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

Thanks for working on this, we're almost there!

Please change the reviewer in your commit message to say "r=jryans".  Please request the next round from me, no need to go through :ntim.

For future patches, please update your patch workflow to add more context lines.  

If you are using hg, check the guide[1].  For example, add to ~/.hgrc:

```
[diff]
git = true
showfunc = true
unified = 8
```

If you are using git, pass something like `-U8` to `git show`.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_customize_the_format_of_the_patches_Mercurial_creates.3F

::: devtools/shared/heapsnapshot/CensusUtils.js
@@ +375,4 @@
>   *
>   * @return {Object<number, TreeNode>}
>   */
> +const createParentMap = function (node, getId = elNode => elNode.id,

What about the more compact format :ntim suggested in comment 8?  It might all fit on one line that way.

::: devtools/shared/heapsnapshot/tests/mochitest/.eslintrc.js
@@ +3,5 @@
> +module.exports = {
> +  // Extend from the shared list of defined globals for mochitests.
> +  "extends": "../../../../.eslintrc.mochitests.js",
> +  "globals": {
> +  	"ChromeUtils": true,

The indentation is odd here, it should be 4 spaces.

::: devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_takeCensus_05.js
@@ +6,4 @@
>  //
>  // Ported from js/src/jit-test/tests/debug/Memory-takeCensus-05.js
>  
> +// eslint-disable-next-line

Why is this needed here?

::: devtools/shared/heapsnapshot/tests/unit/test_ReadHeapSnapshot.js
@@ +2,4 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  // Test that we can read core dumps into HeapSnapshot instances.
> +// eslint-disable-next-line

Why is this needed?

Comment 17

[Tim Nguyen :ntim]

Comment on attachment 8844377 [details] [diff] [review]
fix-1326331-1.patch

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

Here are some extra comments. Please ask Ryan for the next review :)

::: devtools/shared/heapsnapshot/tests/mochitest/test_DominatorTree_01.html
@@ +12,5 @@
>  <body>
>  <pre id="test">
>  <script>
> +"use strict";
> +/* global SimpleTest, window, ChromeUtils, DominatorTree, ok */

You shouldn't need to define `SimpleTest` or `ok` as a global here.

::: devtools/shared/heapsnapshot/tests/unit/Census.jsm
@@ +115,3 @@
>            enter: expectedLeaf,
>            done: expectedLeaf,
>            check: elt => compare(elt, basis)

These 3 lines seem to have the wrong indentation.

::: devtools/shared/heapsnapshot/tests/unit/Match.jsm
@@ +46,2 @@
>  
> +      toString: () => "[object Pattern]"

The indentation in this block seems wrong too.

@@ +212,4 @@
>  
> +  return {
> +    Pattern: Pattern,
> +    MatchError: MatchError

This can be simplfied to:

return {
  Pattern,
  MatchError,
};

::: devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_takeCensus_05.js
@@ +6,4 @@
>  //
>  // Ported from js/src/jit-test/tests/debug/Memory-takeCensus-05.js
>  
> +// eslint-disable-next-line

If this is to workaround the global issue, I would rather do /* eslint-disable use-strict */

/* eslint-disable use-strict */

Comment 18

[Ruturaj Vartak]

Attachment: fix-1326331-2.patch

- shortened the function argument as :ntim had suggested (that I missed!)
- Fixed odd indentation in mochitest/.eslintrc.js (it had a mix of tabs and spaces)
- Removed unwanted global in mochitest/test_DominatorTree_01.html
- Fixed indentation in tests/unit/Census.jsm
- Fixed indentation block in tests/unit/Match.jsm
- Simplified return in function match in tests/unit/Match.jsm
- I've kept the "use strict" suppressions as the previous patch

- used 8 lines for context

Comment 19

[J. Ryan Stinnett [:jryans]]

Comment on attachment 8844865 [details] [diff] [review]
fix-1326331-2.patch

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

It seems like we're quite close, but I'd like to see a reply about what's happening at these lines I marked in my previous review.

::: devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_takeCensus_05.js
@@ +5,5 @@
>  // wrapper GC roots.
>  //
>  // Ported from js/src/jit-test/tests/debug/Memory-takeCensus-05.js
>  
> +// eslint-disable-next-line

:ntim suggested a possible workaround for this, but I'm still not sure I understand what's wrong.  Can you explain what the linting error is here?

::: devtools/shared/heapsnapshot/tests/unit/test_ReadHeapSnapshot.js
@@ +1,5 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  // Test that we can read core dumps into HeapSnapshot instances.
> +// eslint-disable-next-line

I'm still not sure I understand what's wrong.  Can you explain what the linting error is here?

Comment 20

[Ruturaj Vartak]

With the following form of suppression (as suggested by :ntim), the error exists

/* eslint-disable use-strict */

========================================================================
$ ./mach eslint  devtools/shared/heapsnapshot/  --no-ignore
/home/rutu/code/gecko-dev/devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_takeCensus_05.js
  10:1  error  Use the global form of 'use strict'.  strict (eslint)

✖ 1 problem (1 error, 0 warnings)
=========================================================================
Hence I had retained `// eslint-disable-next-line` form of suppression.


Note: I've already run the command `$ ./mach eslint --setup` to ensure I'm at the latest version of eslint.

Comment 21

[Tim Nguyen :ntim]

Yeah, sorry it's /* eslint-disable strict */

Comment 22

[Ruturaj Vartak]

Attachment: fix-1326331-3.patch

- Fixed the "use strict" suppression directives

Comment 23

[J. Ryan Stinnett [:jryans]]

Comment on attachment 8845736 [details] [diff] [review]
fix-1326331-3.patch

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

Looks good to me, thanks for working on it!

Please request ni? if you need help submitting to try.

Comment 24

[Ruturaj Vartak]

I'm sorry for submitting the form before the message, . I'm new to try, I use git and not hg. Could you please help me setup "try". Also the command if any would help.

I'm not sure if following would be the right command

./mach try -b d -p linux64 devtools/shared/heapsnapshot/tests/

Comment 25

[Ruturaj Vartak]

:( I'm sorry goofed up again, please see comment#24

Comment 26

[J. Ryan Stinnett [:jryans]]

(In reply to Ruturaj Vartak from comment #24)
> I'm sorry for submitting the form before the message, . I'm new to try, I
> use git and not hg. Could you please help me setup "try". Also the command
> if any would help.
> 
> I'm not sure if following would be the right command
> 
> ./mach try -b d -p linux64 devtools/shared/heapsnapshot/tests/

With git, you'd need to be using git cinnabar, which can be complex to set up for new contributors.

I'll go ahead and submit the run.  I typically use:

./mach try -b do -p linux64,macosx64,win32,win64 -u xpcshell,mochitests

to be thorough and cover all DevTools tests.  It's possible some files changed will have impact outside just this directory, so best to be safe.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9aa876e2c3ef6c6697aeac47851bf3e695b4350c

Comment 27

[J. Ryan Stinnett [:jryans]]

Try run is looking good to me, I believe this is ready to land.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/476316fd67e9
Fix Eslint errors in devtools/shared/heapsnapshot/. r=jryans

Comment 29

[Ruturaj Vartak]

Thanks :jryans and :ntim !

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/476316fd67e9