Closed
Bug 1326331
Opened 4 years ago
Closed 4 years ago
Fix Eslint errors in devtools/shared/heapsnapshot/
Categories
(DevTools :: General, defect, P3)
DevTools
General
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ntim, Assigned: ruturaj)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file, 3 obsolete files)
145.87 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
Run `./mach eslint devtools/shared/heapsnapshot/ --no-ignore` to see the errors.
Reporter | ||
Updated•4 years ago
|
Blocks: devtools-eslint
Priority: -- → P3
Assignee | ||
Updated•4 years ago
|
Assignee: nobody → ruturaj
Assignee | ||
Comment 1•4 years ago
|
||
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 ?
Flags: needinfo?(jryans)
Reporter | ||
Comment 3•4 years ago
|
||
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?
Flags: needinfo?(ntim.bugs) → needinfo?(sphink)
(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.
Flags: needinfo?(jryans)
Comment 5•4 years ago
|
||
(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.
Flags: needinfo?(sphink)
Assignee | ||
Comment 6•4 years ago
|
||
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)
Attachment #8843570 -
Flags: review?(ntim.bugs)
Reporter | ||
Comment 7•4 years ago
|
||
(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 ?
Flags: needinfo?(jryans)
Reporter | ||
Comment 8•4 years ago
|
||
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;
Attachment #8843570 -
Flags: review?(ntim.bugs) → review?(jryans)
Assignee | ||
Comment 9•4 years ago
|
||
(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.
Reporter | ||
Comment 10•4 years ago
|
||
(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•4 years ago
|
||
(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 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
Attachment #8843570 -
Flags: review?(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.
Flags: needinfo?(jryans)
Assignee | ||
Comment 14•4 years ago
|
||
- 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
Attachment #8843570 -
Attachment is obsolete: true
Attachment #8844377 -
Flags: review?(ntim.bugs)
Reporter | ||
Comment 15•4 years ago
|
||
Comment on attachment 8844377 [details] [diff] [review] fix-1326331-1.patch Review of attachment 8844377 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting review to Ryan
Attachment #8844377 -
Flags: review?(ntim.bugs) → review?(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?
Attachment #8844377 -
Flags: review?(jryans)
Reporter | ||
Comment 17•4 years ago
|
||
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 */
Assignee | ||
Comment 18•4 years ago
|
||
- 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
Attachment #8844377 -
Attachment is obsolete: true
Attachment #8844865 -
Flags: review?(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?
Attachment #8844865 -
Flags: review?(jryans)
Assignee | ||
Comment 20•4 years ago
|
||
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.
Reporter | ||
Comment 21•4 years ago
|
||
Yeah, sorry it's /* eslint-disable strict */
Assignee | ||
Comment 22•4 years ago
|
||
- Fixed the "use strict" suppression directives
Attachment #8844865 -
Attachment is obsolete: true
Attachment #8845736 -
Flags: review?(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.
Attachment #8845736 -
Flags: review?(jryans) → review+
Assignee | ||
Updated•4 years ago
|
Flags: needinfo?(jryans)
Assignee | ||
Comment 24•4 years ago
|
||
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/
Flags: needinfo?(jryans)
Assignee | ||
Comment 25•4 years ago
|
||
:( I'm sorry goofed up again, please see comment#24
Flags: needinfo?(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
Flags: needinfo?(jryans)
Try run is looking good to me, I believe this is ready to land.
Keywords: checkin-needed
Comment 28•4 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/476316fd67e9 Fix Eslint errors in devtools/shared/heapsnapshot/. r=jryans
Keywords: checkin-needed
Assignee | ||
Comment 29•4 years ago
|
||
Thanks :jryans and :ntim !
Comment 30•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/476316fd67e9
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•