1264676 - [rep tests] Add tests for array rep

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P1)

Description

[Lin Clark [:linclark]]

See Bug 1257552

Comment 1

[Lin Clark [:linclark]]

Attachment: 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?

Comment 2

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

(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

Comment 3

[Lin Clark [:linclark]]

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

Comment 4

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

(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

Comment 5

[Lin Clark [:linclark]]

Attachment: Bug 1264676 - [rep tests] Add tests for array rep.

Review commit: https://reviewboard.mozilla.org/r/63488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63488/

Comment 6

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

Comment on attachment 8769745 [details]
Bug 1264676 - [rep tests] Add tests for array rep.

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

LGTM!

Honza

Comment 7

[Lin Clark [:linclark]]

Thanks for the review :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/dcb424a33616
[rep tests] Add tests for array rep. r=Honza

Comment 9

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

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