8

Quote Symbol description when one is provided

 3 years ago
source link: https://bugzilla.mozilla.org/show_bug.cgi?id=1695141
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client
Closed Bug 1695141 Opened 17 days ago Closed 6 days ago

Quote Symbol description when one is provided

Categories

(DevTools :: Shared Components, defect, P3)

Tracking

(firefox88 fixed)

RESOLVED FIXED

88 Branch

Tracking Status firefox88 --- fixed

People

(Reporter: nchevobbe, Assigned: rsawra)

Details

(Keywords: good-first-bug)

Steps to reproduce

  1. Open the console
  2. Evaluate Symbol("hi")

Expected results

The result shows Symbol("hi")

Actual results

The result shows Symbol(hi)

Hi Nicolas,I would like to give it a try.
Can you please help me locate the code?

Hello Rahul, thank you for helping us :)

So I think this should be about setting this property to true devtools/client/shared/components/reps/reps/symbol.js#47 , and then updating all the tests that are going to fail.

There are jest tests for Reps that can be run with

cd devtools/client/shared/components/test/node/
yarn
yarn test

We also have end to end test that might check Symbols too, like https://searchfox.org/mozilla-central/source/devtools/client/webconsole/test/browser/browser_webconsole_object_inspector_symbols.js (that you can run with ./mach test browser_webconsole_object_inspector_symbols.js). Let me know if you have any question.

Assignee: nobody → r.sawra
Status: NEW → ASSIGNED

Hi Nicolas,I tried to reproduce the bug, but i got the actual result as Symbol(hi) and not as Symbol("hi").I am using Nightly version.

Flags: needinfo?(nchevobbe)

So the actual result should be Symbol("hi") and not Symbol(hi).

Hi Nicolas, after changing useQuotes:true running yarn test the following test failed:

components/reps/symbol.test.js
test Symbol with long string › renders the expected content

expect(received).toEqual(expected) // deep equality

Expected: "Symbol(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa…)"
Received: "Symbol(\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa…\")"

     );
  
   expect(renderedComponent.text()).toEqual(
     |                                      ^
      "Symbol(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa…)"
   );
  expectActorAttribute(renderedComponent, stub.actor); `

Could you please explain what is probably happening here?

Hi Nicolas,while debugging the error more i found that the value passed i.e name is checked to be of type longString, you can see here https://searchfox.org/mozilla-central/rev/f1159268add2fd0959e9f91b474f5da74c90f305/devtools/client/shared/components/reps/reps/symbol.js#42, if name is of type longString the output is correct ( for e.g you can test in console, type Symbol("aa".repeat(10000))).
So i wanted to know why name.type === "longString" is explicitly checked in if condition?
If i remove name.type condition it works perfectly fine!

Hello Rahul,

It's expected that tests are failing, since we're changing how the Symbols look.
So here https://searchfox.org/mozilla-central/rev/6a023272d590409c80458d373986e379b3ad86f4/devtools/client/shared/components/test/node/components/reps/symbol.test.js#58 , we should change this to:

    expect(renderedComponent.text()).toEqual(
      `Symbol("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa…")`
    );

(with the description wrapped in quotes)

Flags: needinfo?(nchevobbe)

Hi Nicolas, (In reply to RAHUL SAWRA[:rsawra] from comment #6)

Hi Nicolas,while debugging the error more i found that the value passed i.e name is checked to be of type longString, you can see here https://searchfox.org/mozilla-central/rev/f1159268add2fd0959e9f91b474f5da74c90f305/devtools/client/shared/components/reps/reps/symbol.js#42, if name is of type longString the output is correct ( for e.g you can test in console, type Symbol("aa".repeat(10000))).
So i wanted to know why name.type === "longString" is explicitly checked in if condition?
If i remove name.type condition it works perfectly fine!

Hi Nicolas, just changing useQuotes:true doesn't seems to be working.
What are your thoughts on above comment where longString seems to be working fine, we just need to figure out for other Strings, you can checkout the jest test for other strings. https://searchfox.org/mozilla-central/rev/6a023272d590409c80458d373986e379b3ad86f4/devtools/client/shared/components/test/node/components/reps/symbol.test.js#27

Flags: needinfo?(nchevobbe)

(In reply to RAHUL SAWRA[:rsawra] from comment #6)

Hi Nicolas,while debugging the error more i found that the value passed i.e name is checked to be of type longString, you can see here https://searchfox.org/mozilla-central/rev/f1159268add2fd0959e9f91b474f5da74c90f305/devtools/client/shared/components/reps/reps/symbol.js#42, if name is of type longString the output is correct ( for e.g you can test in console, type Symbol("aa".repeat(10000))).
So i wanted to know why name.type === "longString" is explicitly checked in if condition?
If i remove name.type condition it works perfectly fine!

Ah sorry, I read your comment too quick.
Yes, I think we could only check if (name) , so we would handle all the cases

Flags: needinfo?(nchevobbe)

Yes (In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)

(In reply to RAHUL SAWRA[:rsawra] from comment #6)

Hi Nicolas,while debugging the error more i found that the value passed i.e name is checked to be of type longString, you can see here https://searchfox.org/mozilla-central/rev/f1159268add2fd0959e9f91b474f5da74c90f305/devtools/client/shared/components/reps/reps/symbol.js#42, if name is of type longString the output is correct ( for e.g you can test in console, type Symbol("aa".repeat(10000))).
So i wanted to know why name.type === "longString" is explicitly checked in if condition?
If i remove name.type condition it works perfectly fine!

Ah sorry, I read your comment too quick.
Yes, I think we could only check if (name) , so we would handle all the cases

Yes, this should work. Thank you for your quick reply Nicolas :). I will update you soon.

Hi Nicolas,
it seems to be working fine now.
There are 2 jest tests failing at

Expected: "Object { x: 10, Symbol(): \"first unnamed symbol\", Symbol(): \"second unnamed symbol\", Symbol(named): \"named symbol\", Symbol(Symbol.iterator): () }"
Received: "Object { x: 10, Symbol(): \"first unnamed symbol\", Symbol(): \"second unnamed symbol\", Symbol(\"named\"): \"named symbol\", Symbol(\"Symbol.iterator\"): () }"

This is in reference to first test.Can you guide me as to how should i make these tests pass?

Flags: needinfo?(nchevobbe)

Hi Nicolas , did you get a chance to see the above tests?

Hello Rahul,

You need to update the test as we now do wrap the description in quotes (Symbol(named) => Symbol("named"))

Seeing that, I think there are some name we don't want to wrap: Symbol.iterator and Symbol.asyncIterator

Flags: needinfo?(nchevobbe)

I think that's because of this: https://searchfox.org/mozilla-central/source/devtools/client/shared/components/reps/reps/symbol.js#69
Now that the text is always a StringRep, we are trying to set the title attribute with it. And since title should be a string, the StringRep gets stringified (hence the [object Object] part).

So for that part, we could try to call https://searchfox.org/mozilla-central/source/devtools/client/shared/components/reps/reps/rep-utils.js#129-141 with object.name instead of trying to reuse symbolText

Flags: needinfo?(nchevobbe)

Hi Nicolas,
so i tried to call https://searchfox.org/mozilla-central/source/devtools/client/shared/components/reps/reps/symbol.js#63 with object.name instead of symbolText .

const config = getElementConfig(
      {
        shouldRenderTooltip,
        className,
        name,
      },
      object
    );

So instead of reusing symbolText, i directly used name, and this works fine,all the jest test cases are passing.

Flags: needinfo?(nchevobbe)
You need to log in before you can comment on or make changes to this bug.

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK