Closed
Bug 1264676
Opened 9 years ago
Closed 8 years ago
[rep tests] Add tests for array rep
Categories
(DevTools :: Shared Components, enhancement, P1)
DevTools
Shared Components
Tracking
(firefox50 fixed)
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: linclark, Assigned: linclark)
References
Details
(Whiteboard: [devtools-html])
Attachments
(1 file, 1 obsolete file)
See Bug 1257552
Updated•9 years ago
|
Severity: normal → enhancement
Whiteboard: [devtools-html]
Updated•9 years ago
|
Blocks: devtools-html-2
Updated•8 years ago
|
Flags: qe-verify-
Priority: -- → P2
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → lclark
Assignee | ||
Comment 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
(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)
Assignee | ||
Comment 3•8 years ago
|
||
(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)
Comment 4•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Updated•8 years ago
|
Iteration: --- → 50.2 - Jul 4
Priority: P2 → P1
Updated•8 years ago
|
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63488/
Attachment #8769745 -
Flags: review?(odvarko)
Assignee | ||
Updated•8 years ago
|
Attachment #8765496 -
Attachment is obsolete: true
Blocks: 1286186
Comment 6•8 years ago
|
||
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+
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
Comment 9•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•