1286186 - Reps: Test that array indexes are sorted as numbers

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Changing priority to match original bug.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1286186-test-array-index.diff

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 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1286186-test-array-index.diff

Created new DOM panel test.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8770946 [details] [diff] [review]
1286186-test-array-index.diff

Patrick can you please review the eslint changes?

Honza

Comment 7

[Patrick Brosset <:pbro>]

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.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1286186-test-array-index.diff

Removed browser-fullScreen.js fix.

@pbrosset: Can you review the eslint parts... very simple changes.

Comment 9

[Patrick Brosset <:pbro>]

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.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Honza

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(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.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1286186-test-array-index.diff

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

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8771438 [details] [diff] [review]
1286186-test-array-index.diff

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

Nice!

Honza

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1286186-test-array-index.diff

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Rebased

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b9c789ba01a0
Reps: Test that array indexes are sorted as numbers. r=honza

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Another mid-air collision... updating.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1286186-test-array-index.diff

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

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Should be fine to land this again.

Comment 24

[Pulsebot]

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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b1fce35d1cc8