Closed
Bug 1264693
Opened 9 years ago
Closed 8 years ago
[rep tests] Add tests for object 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 | ||
Comment 1•8 years ago
|
||
Under what circumstances would Rep return the Object rep? For any objects that the console is working with, I think it would always result in the Grip rep.
Flags: needinfo?(odvarko)
Comment 2•8 years ago
|
||
Yes, the Console never uses/renders JS objects directly and only access it remotely through grips.
I don't know if this counts, but: e.g. The HTTP Inspector uses it for rendering JSON HTTP responses. JSON HTTP response is parsed into an object on the client side and rendered using Object rep.
Honza
Flags: needinfo?(odvarko)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → lclark
Assignee | ||
Comment 3•8 years ago
|
||
It looks like this is still outputing Object{props} instead of having a space between the title and props, e.g. Object {props}. It also looks like the properties are being reordered in an unexpected way in testNested (unless I misunderstand).
I think both of those can be addressed in follow-ups.
Attachment #8765546 -
Flags: review?(odvarko)
Updated•8 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 50.2 - Jul 4
Priority: P2 → P1
Comment 4•8 years ago
|
||
Comment on attachment 8765546 [details] [diff] [review]
Bug1264693.patch
Review of attachment 8765546 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me. There is one comment, please fix, otherwise R+
Thanks Lin for doing these tests!
Honza
::: devtools/client/shared/components/test/mochitest/test_reps_object.html
@@ +158,5 @@
> + arr: [2]
> + },
> + strProp: "test string",
> + arrProp: [1]
> + };
Please put at least 4 items into the `arr` and `arrProp`. In such case we can also cover what mode is actually used for inner objects/arrays. And let it pass...
Currently Grip rep and GripArray rep are both using 'tiny' for inner props/items, but Obj rep and Array rep are not doing this. I am not entirely sure what's the right approach here. Using 'tiny' seems to restrictive 'short' renders more info. But at least we should be consistent (across local and remote objects).
The difference is as follows:
An example object:
let obj = {
p1: "John",
p2: [1,2,3,4],
p3: true
}
// Inner items are using 'tiny' - see the `p2` array
Object {p1: "John", p3: true, p2: [3]}
// Inner items are not using 'tiny' - see the `p2` array
// But it's using 'long' mode!
Object {p1: "John", p3: true, p2: [1, 2, 3, 4]}
Please create a report for this, the inner mode should definitely not be 'long'. I'll fix it and adopt this test.
Honza
Attachment #8765546 -
Flags: review?(odvarko) → review+
Comment 5•8 years ago
|
||
(In reply to Lin Clark [:linclark] from comment #3)
> Created attachment 8765546 [details] [diff] [review]
> Bug1264693.patch
>
> It looks like this is still outputing Object{props} instead of having a
> space between the title and props, e.g. Object {props}.
Please report it and I'll fix it (and I'll also adopt this test)
> It also looks like
> the properties are being reordered in an unexpected way in testNested
> (unless I misunderstand).
This is what I explained here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1264676#c2
... and yes it should be fixed. Please make sure there is a bug reported and I can fix it.
This ordering issue might be covered in this test (not by test for the Array rep). I'll let you to decide.
> I think both of those can be addressed in follow-ups.
Yes
Honza
Updated•8 years ago
|
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63704/
Attachment #8770139 -
Flags: review?(odvarko)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8770139 [details]
Bug 1264693 - [rep tests] Add tests for object rep.
https://reviewboard.mozilla.org/r/63704/#review60654
Carrying over review from patch
Attachment #8770139 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8770139 -
Flags: review?(odvarko)
Assignee | ||
Updated•8 years ago
|
Attachment #8765546 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/fcb309979c71
[rep tests] Add tests for object 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
•