Closed Bug 1264676 Opened 4 years ago Closed 4 years ago

[rep tests] Add tests for array rep

Categories

(DevTools :: Shared Components, enhancement, P1)

enhancement

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox50 --- fixed

People

(Reporter: linclark, Assigned: linclark)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 1 obsolete file)

See Bug 1257552
Blocks: 1257552
Severity: normal → enhancement
Whiteboard: [devtools-html]
Flags: qe-verify-
Priority: -- → P2
Assignee: nobody → lclark
Attached patch Bug1264676.patch (obsolete) — Splinter Review
This patch is mostly ready. However, the last test fails in a way I wouldn't expect. It treats p2 as uninteresting. Honza, does this sound right to you?
Flags: needinfo?(odvarko)
(In reply to Lin Clark [:linclark] from comment #1)
> Created attachment 8765496 [details] [diff] [review]
> Bug1264676.patch
> 
> This patch is mostly ready. However, the last test fails in a way I wouldn't
> expect. It treats p2 as uninteresting. Honza, does this sound right to you?
Yes, this is expected.

The `stub` array (in testNested) has one item: an object. The object is rendered using `Obj` rep and this rep filters displayed props using `isInterestingProp`. Preferred props are: boolean, number and non-empty string. Since p2 is an array it is *not* displayed. Other props are displayed only if there is not enough interesting props and the max limit wasn't reached.

There is one case we should fix (perhaps we already have a bug report?). Imagine the following object:


let obj = {
  p1: "John"    // Interesting prop
  p2: [1,2,3],  // Not interesting prop
  p3: true      // Interesting prop
}

Rendered object would look like as follows:
Object{p1: "John", p3: true, p2: [1, 2, 3]}

(btw. there is no space between 'Object' and '{', but I'll get back to this in bug 1264693)

Note that the array is rendered as the last item even if defined as the second. This is because the rep renders all interesting props first and consequently not-interesting props are rendered till the max limit is reached. I think that we should have a bug report and fix it.

This test should just fix the `stub` arrays (missing p2) and cover the case (above) where not-interesting props is at the end (and passing). You might also want to point to the related but report (if it already exists) so, we can fix the test as soon as the order is fixed.

Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #2)

> Preferred props are: boolean, number and non-empty string.

Was there a particular reason why array is not a preferred prop? If I saw this in the wild, it would be surprising to me and I would think the object had dropped that prop. It might be worth getting Helen's opinion on this. Maybe we should start a Reps UX issue to track all of these questions we need to ask her.

> There is one case we should fix (perhaps we already have a bug report?).
> Imagine the following object:
> 
> 
> let obj = {
>   p1: "John"    // Interesting prop
>   p2: [1,2,3],  // Not interesting prop
>   p3: true      // Interesting prop
> }

Ah yes, I did notice this in the object test, and assumed it was related. FYI, that test will break and need to have props rearranged when we fix this.

Created Bug 1282791 to track that.
Flags: needinfo?(odvarko)
(In reply to Lin Clark [:linclark] from comment #3)
> (In reply to Jan Honza Odvarko [:Honza] from comment #2)
> 
> > Preferred props are: boolean, number and non-empty string.
> 
> Was there a particular reason why array is not a preferred prop?
It's because it takes relatively a lot of space comparing to number, boolean and (cropped) string. The point here was to be able to quickly identify an object by seeing just a few (3 in the short mode) properties. Another example, seeing: Object {null, null, null, more...} is not helpful.

> If I saw
> this in the wild, it would be surprising to me and I would think the object
> had dropped that prop.
Don't forget that there is 'more...' displayed so, the user can inspect the entire object if not all properties are visible. Anyway, I am open, we might want to make array preferred and make sure it uses the 'short' mode and not taking too much space. 

> It might be worth getting Helen's opinion on this.
> Maybe we should start a Reps UX issue to track all of these questions we
> need to ask her.
Yes, good point.

> 
> > There is one case we should fix (perhaps we already have a bug report?).
> > Imagine the following object:
> > 
> > 
> > let obj = {
> >   p1: "John"    // Interesting prop
> >   p2: [1,2,3],  // Not interesting prop
> >   p3: true      // Interesting prop
> > }
> 
> Ah yes, I did notice this in the object test, and assumed it was related.
> FYI, that test will break and need to have props rearranged when we fix this.
> 
> Created Bug 1282791 to track that.
Great, thanks!

Honza
Flags: needinfo?(odvarko)
Status: NEW → ASSIGNED
Iteration: --- → 50.2 - Jul 4
Priority: P2 → P1
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Attachment #8765496 - Attachment is obsolete: true
Comment on attachment 8769745 [details]
Bug 1264676 - [rep tests] Add tests for array rep.

https://reviewboard.mozilla.org/r/63488/#review60612

LGTM!

Honza
Attachment #8769745 - Flags: review?(odvarko) → review+
Thanks for the review :)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/dcb424a33616
[rep tests] Add tests for array rep. r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dcb424a33616
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.