Closed Bug 1264693 Opened 4 years ago Closed 4 years ago

[rep tests] Add tests for object 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
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)
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: nobody → lclark
Attached patch Bug1264693.patch (obsolete) — Splinter Review
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)
Status: NEW → ASSIGNED
Iteration: --- → 50.2 - Jul 4
Priority: P2 → P1
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+
(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
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
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+
Attachment #8770139 - Flags: review?(odvarko)
Attachment #8765546 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/fcb309979c71
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.