1695141 - Quote Symbol description when one is provided

Nicolas Chevobbe [:nchevobbe]

Reporter

Description

•

3 months ago

Steps to reproduce

Open the console
Evaluate Symbol("hi")

Expected results

The result shows Symbol("hi")

Actual results

The result shows Symbol(hi)

RAHUL SAWRA[:rsawra]

Assignee

Comment 1

•

3 months ago

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

Nicolas Chevobbe [:nchevobbe]

Reporter

Comment 2

•

3 months ago

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

Nicolas Chevobbe [:nchevobbe]

Reporter

Updated

•

3 months ago

Status: NEW → ASSIGNED

RAHUL SAWRA[:rsawra]

Assignee

Comment 3

•

3 months ago

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)

RAHUL SAWRA[:rsawra]

Assignee

Comment 4

•

3 months ago

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

RAHUL SAWRA[:rsawra]

Assignee

Comment 5

•

3 months ago

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?

RAHUL SAWRA[:rsawra]

Assignee

Comment 6

•

3 months ago

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!

Nicolas Chevobbe [:nchevobbe]

Reporter

Comment 7

•

3 months ago

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)

RAHUL SAWRA[:rsawra]

Assignee

Comment 8

•

3 months ago

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)

Nicolas Chevobbe [:nchevobbe]

Reporter

Comment 9

•

3 months ago

(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)

RAHUL SAWRA[:rsawra]

Assignee

Comment 10

•

3 months ago

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.

RAHUL SAWRA[:rsawra]

Assignee

Comment 11

•

3 months ago

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)

RAHUL SAWRA[:rsawra]

Assignee

Comment 12

•

3 months ago

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

Nicolas Chevobbe [:nchevobbe]

Reporter

Comment 13

•

3 months ago

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)

RAHUL SAWRA[:rsawra]

Assignee

Comment 14

•

3 months ago

Hi Nicolas,
1 test case fails continously and i am little bit confused here :(

https://searchfox.org/mozilla-central/rev/6a023272d590409c80458d373986e379b3ad86f4/devtools/client/shared/components/test/node/components/reps/symbol.test.js#27

Expected: "Symbol(foo)"
Received: "Symbol([object Object])"

Flags: needinfo?(nchevobbe)

Nicolas Chevobbe [:nchevobbe]

Reporter

Comment 15

•

3 months ago

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)

RAHUL SAWRA[:rsawra]

Assignee

Comment 16

•

3 months ago

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)

Nicolas Chevobbe [:nchevobbe]

Reporter

Comment 17

•

3 months ago

great, you can commit (https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-write-a-patch) and push the patch to review (https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-submit-a-patch)

Flags: needinfo?(nchevobbe)

RAHUL SAWRA[:rsawra]

Assignee

Comment 18

•

3 months ago

Attached file Bug 1695141 - Quote Symbol description when one is provided .r?nchevobbe — Details

Pulsebot

Comment 19

•

3 months ago

Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b332567cbbca
Quote Symbol description when one is provided .r=nchevobbe

Natalia Csoregi [:nataliaCs]

Comment 20

•

3 months ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/b332567cbbca

Status: ASSIGNED → RESOLVED

Closed: 3 months ago

status-firefox88: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 88 Branch

Bugzilla

Quote Symbol description when one is provided

Categories

(DevTools :: Shared Components, defect, P3)

Tracking

(firefox88 fixed)

People

(Reporter: nchevobbe, Assigned: rsawra)

References

Details

(Keywords: good-first-bug)

Crash Data

Security

(public)

User Story

Attachments

(1 file)

Description

Comment 1

Comment 2

Updated

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Comment 14

Comment 15

Comment 16

Comment 17

Comment 18

Comment 19

Comment 20