Bug 1281047 fixed an issue where array indexes were sorted as strings instead of numbers. devtools/client/shared/components/test/mochitest/test_reps_array.html should be updated to test this change.

Changing priority to match original bug.

Apart from the testArray method this patch turns on eslint for the test... there is no point splitting the patch as it is plenty small enough to understand. We don't need to test the tree as it preserves order so if the order is correct in this test it will be correct in the tree.

Comment on attachment 8770094 [details] [diff] [review] 1286186-test-array-index.diff Review of attachment 8770094 [details] [diff] [review]: ----------------------------------------------------------------- The test passes for me even if I don't have the patch from bug 1281047 applied. Also, I am not sure how the test should actually verify the fixed issue. The test applies on Array rep object, not on GripArray, which is used in the DOM panel (ArrayRep is for local JS array that are directly accessible, GripArray is for remote array objects that are accessible through RDP/grips) and also it doesn't test children objects that appears if the user expands an array (these children are fetched using 'getPrototypeAndProperties' packet). My feeling is that since the fix is part of the DOM panel, the test should also be part of it (there are some existing DOM panel tests that can be used as an inspiration). Honza

Created new DOM panel test.

Comment on attachment 8770946 [details] [diff] [review] 1286186-test-array-index.diff Review of attachment 8770946 [details] [diff] [review]: ----------------------------------------------------------------- Looks and works nice! Just one inline comment to resolve. I am not sure about the Eslint changes so, I'll ask Patrick for feedback. Thank Mike, Honza ::: devtools/client/dom/test/head.js @@ +107,5 @@ > + let doc = panel.panelWin.document; > + let nodes = [...doc.querySelectorAll(".treeLabel")]; > + > + // Find the label (object name) for which we want the children. > + while (true) { Please make a comment explaining that this part of the logic removes nodes from the array and the reset of the array is used in the second part. @@ +122,5 @@ > + } > + > + // Now get the children. > + for (node of nodes) { > + if (node.parentElement.getAttribute("style") !== "padding-left:0px;") { Checking the style attribute can break easily. It would be safer to create `window._b` variable and the nodes till node.textContetnt != `_b`. Honza

Comment on attachment 8770946 [details] [diff] [review] 1286186-test-array-index.diff Patrick can you please review the eslint changes? Honza

Comment on attachment 8770946 [details] [diff] [review] 1286186-test-array-index.diff Review of attachment 8770946 [details] [diff] [review]: ----------------------------------------------------------------- Just one comment about the ESLint changes. ::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/import-browserjs-globals.js @@ +34,5 @@ > "browser/base/content/browser-ctrlTab.js", > "browser/base/content/browser-customization.js", > "browser/base/content/browser-devedition.js", > "browser/base/content/browser-feeds.js", > + "browser/base/content/browser-fullScreenAndPointerLock.js", Hmm, I think this is unrelated and is actually tracked in bug 1285936. You should get rid of this change as I think it's better to handle the change in bug 1285936 since it seems like it's causing other problems.

Removed browser-fullScreen.js fix. @pbrosset: Can you review the eslint parts... very simple changes.

Comment on attachment 8771395 [details] [diff] [review] 1286186-test-array-index.diff Review of attachment 8771395 [details] [diff] [review]: ----------------------------------------------------------------- ESLint changes look good to me.

Sorry to remove the flag, but what about my review comments? Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #10) > Sorry to remove the flag, but what about my review comments? > > Honza Oops, completely missed them. (In reply to Jan Honza Odvarko [:Honza] from comment #5) > Comment on attachment 8770946 [details] [diff] [review] > 1286186-test-array-index.diff > > Review of attachment 8770946 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks and works nice! > > Just one inline comment to resolve. > > I am not sure about the Eslint changes so, I'll ask Patrick for feedback. > > Thank Mike, > Honza > > ::: devtools/client/dom/test/head.js > @@ +107,5 @@ > > + let doc = panel.panelWin.document; > > + let nodes = [...doc.querySelectorAll(".treeLabel")]; > > + > > + // Find the label (object name) for which we want the children. > > + while (true) { > > Please make a comment explaining that this part of the logic removes nodes > from the array and the reset of the array is used in the second part. > Done. > @@ +122,5 @@ > > + } > > + > > + // Now get the children. > > + for (node of nodes) { > > + if (node.parentElement.getAttribute("style") !== "padding-left:0px;") { > > Checking the style attribute can break easily. > > It would be safer to create `window._b` variable and the nodes till > node.textContetnt != `_b`. > > Honza That would work in this case but then getAllRowsForLabel() would only work in this case. I will try to find another generic method of getting just the children of a particular property.

We now set a level on each tree node as discussed.

Comment on attachment 8771438 [details] [diff] [review] 1286186-test-array-index.diff Review of attachment 8771438 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Honza

has problems to apply: Hunk #1 FAILED at 8 1 out of 2 hunks FAILED -- saving rejects to file devtools/client/shared/components/test/mochitest/test_reps_array.html.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh 1286186-test-array-index.diff

Rebased

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/b9c789ba01a0 Reps: Test that array indexes are sorted as numbers. r=honza

Backed out in https://hg.mozilla.org/integration/fx-team/rev/24530e316b12 for bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=10678429&repo=fx-team

Another mid-air collision... updating.

Fixed... let's run this through try again before landing it.

Should be fine to land this again.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/b1fce35d1cc8 Reps: Test that array indexes are sorted as numbers; r=honza