See Bug 1257552

I decided to test textContent instead of the HTML for this. This led me to realize that it's not working as expected in the test. For example, let's say you have the following object which you are evaluating: > var obj = {objProp: {id: 1}, strProp: "test string"} The current console outputs this: > Object { objProp: Object, strProp: "test string" } However, when I use the grip with the grip rep, it gives me this. > {objProp: {type: "object", class: "Object", actor: "server1.conn0.obj146", more...}, strProp: "test string"} The nested object is showing its metadata instead of its actual props. I don't think this is how it's working in the DOM panel, though. Honza, is there something that I need to mock out in the RDP code so that this works?

(In reply to Lin Clark [:linclark] from comment #1) > The current console outputs this: > Object { objProp: Object, strProp: "test string" } Btw. the Reps are currently using `{}` as representation of JS object so, the rendering in the DOM panel looks like as follows: Object { objProp: {}, strProp: "test string" } I like it a bit better since it's closer to JavaScript syntax, but Chrome is also using 'Object' so, it might be better to keep it for consistency. Anyway, this can easily be changed. The problem is that the inner `objProp` prop is fully rendered (with properties) while it should use 'tiny' mode and render only `{}` (or `Object`) > The nested object is showing its metadata instead of its actual props. So, yes, that's the issue - the nested object should *not* show its metadata. But, it can't show the actual props since those are not available in the grip's preview (i.e. in your `gripStub`). The preview is only 1-level deep and there are no actual properties of the nested object. > working in the DOM panel, though. Honza, is there something that I need to mock out in the RDP code so > that this works? Yes There are two things we should do: 1) The test should specify a 'default' rep. The default rep is a rep used for objects that don't have its own specific rep. It's usually generic rep for rendering generic objects: either `Obj` rep (local JS object) or `Grip` rep (remote JS object). As you pointed out in the other bug 1264693, the Obj rep isn't needed for the Console panel since it's only rendering remote objects, but there are valid cases in our code-base that require it (e.g. rendering JSON). Currently it's the `Obj` rep, which is set as the default one and it doesn't support 'tiny' mode. I reported this as bug 1275556. The 'tiny' mode is automatically set for nested props so, they don't render their props (which are not available as explained above) 2) However, the test should set the 'Grip' rep as the default [1] (which is supporting the 'tiny' mode). const renderedRep = shallowRenderComponent(Rep, { object: gripStub, defaultRep: Grip }); const renderedComponent = renderComponent(Grip.rep, { object: gripStub, defaultRep: Grip }); And it'll fix the problem. Honza [1] Now I am thinking that the test is not just for the Console panel (which doesn't need Obj rep) so, setting Grip rep as the default one might be actually a workaround till bug 1275556 is fixed.

Honza, this patch is almost ready but just a couple of questions for you: 1. please check out the testUninterestingProps test in the patch. Is this how uninteresting props are expected to work? 2. I'm planning to test the different modes throughout these tests. Besides that, can you think of anything else that should be tested?

(In reply to Lin Clark [:linclark] from comment #3) > Created attachment 8756879 [details] [diff] [review] > Bug1264685.patch > > Honza, this patch is almost ready but just a couple of questions for you: > > 1. please check out the testUninterestingProps test in the patch. Is this > how uninteresting props are expected to work? Nope, the expected result is: {c: "c", d: 1, a: undefined, more...} And the undefined `a` prop is displayed only because there are no other better props to display (and 3 is the max). The problem is that isInterestingProp includes also `Object` type. I think that we should remove Object from interesting props since we only display `Object` label anyways (no nested props), which isn't much useful to the user (actual primitive values represents better overview of the object). Also the logic of displaying props should be improved. In case when there are not enough 'interesting' props and we display also 'uninteresting' props we should preserve the right order. Currently the a: undefined is at the end, while it should actually be at the beginning. > 2. I'm planning to test the different modes throughout these tests. Besides > that, can you think of anything else that should be tested? I think the test is pretty thorough. The only thing is that most of code is taken by grips definition. Would that make sense to move it out into separate file and share across tests? Honza

Honza, I was wondering whether you could clarify how long mode is supposed to work. For all of my tests, it seems to do the same thing as short mode.

(In reply to Lin Clark [:linclark] from comment #5) > Honza, I was wondering whether you could clarify how long mode is supposed > to work. For all of my tests, it seems to do the same thing as short mode. The long mode is supported by e.g. ArrayRep (for local array objects, e.g. when parsing JSON) and GripArray rep (for remote array objects). This mode is supposed to be used e.g. in the Console panel when the user logs an array object. Here are some other cases: 'tiny' - Displaying an object as an individual item inside an array. For example, an object would be rednered as 'Object', an array would be rendered as '[<length>]' (where <length> is size of the array). 'short' - Displaying an object in a tree (e.g. in the DOM panel). For example, an array would render only 3 items max 'long' - Displaying an array inside the Console panel. For example, an array can render up to 100 items. There is enough space in the Console panel and rendering an array can easily span multiple rows. --- Support 'short' and 'long' modes for Grip/Obj reps can be similar, the difference is mostly number of properties displayed (e.g. 3/100 max) Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #6) > Support 'short' and 'long' modes for Grip/Obj reps can be similar, the > difference is mostly number of properties displayed (e.g. 3/100 max) Based on this I would expect the testMoreThanMaxProps test that I have in this aptch to fail, but it passes. Can you double check this as part of the review and make sure it's working as you expect? If that is working as expected, then I think this is ready.

Comment on attachment 8763510 [details] [diff] [review] Bug1264685.patch Review of attachment 8763510 [details] [diff] [review]: ----------------------------------------------------------------- The short & long modes are working ok for me. All failures I am seeing are caused by a space. See my inline comments. Otherwise it looks good, I like the way it's written! I am giving - just to make sure we resolve the space problem. Honza ::: devtools/client/shared/components/test/mochitest/test_reps_grip.html @@ +15,5 @@ > +<script src="head.js" type="application/javascript;version=1.8"></script> > +<script type="application/javascript;version=1.8"> > +window.onload = Task.async(function* () { > + let ReactDOM = browserRequire("devtools/client/shared/vendor/react-dom"); > + let React = browserRequire("devtools/client/shared/vendor/react"); Can be removed. @@ +74,5 @@ > + function testMaxProps() { > + // Test object: `{a: "a", b: "b", c: "c"}`; > + const testName = "testMaxProps"; > + > + const defaultOutput = `Object{a: "a", b: "b", c: "c"}`; The current implementation creates a space between `Object` and `{`. That's why this test fails (there are other places in the test). I think we decided to have no space. In any case this should be resolved before the test lands. @@ +102,5 @@ > + function testMoreThanMaxProps() { > + // Test object = `{a: "a", b: "b", c: "c", d: "d", e: "e"}` > + const testName = "testMoreThanMaxProps"; > + > + const defaultOutput = `Object{a: "a", b: "b", c: "c", more...}`; Fails because of space. @@ +137,5 @@ > + function testNestedObject() { > + // Test object: `{objProp: {id: 1}, strProp: "test string"}` > + const testName = "testNestedObject"; > + > + const defaultOutput = `Object{objProp: Object, strProp: "test string"}`; Fails because of space. @@ +165,5 @@ > + function testNestedArray() { > + // Test object: `{arrProp: ["foo", "bar", "baz"]}` > + const testName = "testNestedArray"; > + > + const defaultOutput = `Object{arrProp: [3]}`; Fails because of space. @@ +192,5 @@ > + > + function testRenderingInMode(modeTests, testName) { > + modeTests.forEach(({mode, expectedOutput, message}) => { > + const modeString = typeof mode === "undefined" ? "no mode" : mode; > + if (!message) message = `${testName}: ${modeString} renders correctly.` Nit: we use block scope for if statement + there is no semicolon at the end. Can we use eslint for .html?

Lin, you are right, the Grid rep doesn't render more than 3 properties in long mode and it should render 100 (max). The longIterator is never used. I've filled bug 1281489. Do you want to look at it? Honza

Thanks, I'll add a link to that bug as a comment in the test to avoid confusion later on.

Ok, rebased to get the latest output formatting and added the comment.

Comment on attachment 8764552 [details] [diff] [review] Bug1264685.patch Review of attachment 8764552 [details] [diff] [review]: ----------------------------------------------------------------- Greate, passes for me now. Please yet remove the React and ReactDOM module deps and fix the if statement (use block scope) so, the code style is consistent. Thanks! Honza

Fixed, thanks for catching that!

seems need also rebasing

Rebased to resolve the conflict in chrome.ini

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/9726914bb35f [rep tests] Add tests for grip rep. r=Honza