Reps: Test that array indexes are sorted as numbers

RESOLVED FIXED in Firefox 50

Status

()

Firefox
Developer Tools: Shared Components
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: miker, Assigned: miker)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 50
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [reserve-html])

Attachments

(1 attachment, 5 obsolete attachments)

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.
Status: NEW → ASSIGNED
Whiteboard: [devtools-html]
Changing priority to match original bug.
Priority: P2 → P1
Created attachment 8770094 [details] [diff] [review]
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.
Attachment #8770094 - Flags: review?(odvarko)
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
Attachment #8770094 - Flags: review?(odvarko)
Blocks: 1263741
Flags: qe-verify-
Priority: P1 → P3
Whiteboard: [devtools-html] → [reserve-html]
Created attachment 8770946 [details] [diff] [review]
1286186-test-array-index.diff

Created new DOM panel test.
Attachment #8770094 - Attachment is obsolete: true
Attachment #8770946 - Flags: review?(odvarko)
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
Attachment #8770946 - Flags: review?(odvarko)
Comment on attachment 8770946 [details] [diff] [review]
1286186-test-array-index.diff

Patrick can you please review the eslint changes?

Honza
Attachment #8770946 - Flags: review?(pbrosset)
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.
Attachment #8770946 - Flags: review?(pbrosset)
Created attachment 8771395 [details] [diff] [review]
1286186-test-array-index.diff

Removed browser-fullScreen.js fix.

@pbrosset: Can you review the eslint parts... very simple changes.
Attachment #8770946 - Attachment is obsolete: true
Attachment #8771395 - Flags: review?(pbrosset)
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.
Attachment #8771395 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
Sorry to remove the flag, but what about my review comments?

Honza
Keywords: checkin-needed
Flags: needinfo?(mratcliffe)
(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.
Flags: needinfo?(mratcliffe)
Created attachment 8771438 [details] [diff] [review]
1286186-test-array-index.diff

We now set a level on each tree node as discussed.
Attachment #8771395 - Attachment is obsolete: true
Attachment #8771438 - Flags: review?(odvarko)
Comment on attachment 8771438 [details] [diff] [review]
1286186-test-array-index.diff

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

Nice!

Honza
Attachment #8771438 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
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
Flags: needinfo?(mratcliffe)
Keywords: checkin-needed
Created attachment 8772727 [details] [diff] [review]
1286186-test-array-index.diff
Attachment #8771438 - Attachment is obsolete: true
Flags: needinfo?(mratcliffe)
Attachment #8772727 - Flags: review+
Rebased
Keywords: checkin-needed
Priority: P3 → P2

Comment 17

a year ago
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
Keywords: checkin-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
Flags: needinfo?(mratcliffe)
Another mid-air collision... updating.
Flags: needinfo?(mratcliffe)
Created attachment 8773271 [details] [diff] [review]
1286186-test-array-index.diff

Fixed... let's run this through try again before landing it.
Attachment #8772727 - Attachment is obsolete: true
Attachment #8773271 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4eb8af8a02c
Blocks: 1288348
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2de6e02ad7b
Should be fine to land this again.
Keywords: checkin-needed

Comment 24

a year ago
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
Keywords: checkin-needed

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b1fce35d1cc8
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.4 - Aug 1
Priority: P2 → P1
You need to log in before you can comment on or make changes to this bug.