1363307 - Add a warning when Console.timeEnd(label) is used with an unknown label

Status: VERIFIED FIXED

(DevTools :: Console, enhancement)

Description

[Andrea Marchesini [:baku]]

> console.timeEnd('a');

we should warn the user the 'a' in an unknown timer label.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: console2.patch

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8865786 [details] [diff] [review]
console2.patch

Review of attachment 8865786 [details] [diff] [review]:
-----------------------------------------------------------------

The tests are a little tricky to write, so I will attach a patch that adds coverage for this and also Bug 1363288 here

::: devtools/client/locales/en-US/webconsole.properties
@@ +95,5 @@
>  Autocomplete.blank=  <- no result
>  
>  maxTimersExceeded=The maximum allowed number of timers in this page was exceeded.
>  timerAlreadyExists=Timer %S already exists.
> +timerDoesntExist=Timer %S doen't exist.

Typo: doesn't

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: tests-webconsole-time.patch

Nicolas, this has a quick fix for the mocha tests (which are currently failing after the sourcemap changes) in addition to covering some new timer errors

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8866048 [details] [diff] [review]
tests-webconsole-time.patch

Review of attachment 8866048 [details] [diff] [review]:
-----------------------------------------------------------------

One question about the fix for sourceMap and a comment about how we display timer name , but aside from that, everything looks good

::: devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js
@@ +146,5 @@
> +    it("shows an error if called again", () => {
> +      const message = stubPreparedMessages.get("timerAlreadyExists");
> +      const wrapper = render(ConsoleApiCall({ message, serviceContainer }));
> +
> +      expect(wrapper.find(".message-body").text()).toBe("Timer bar already exists.");

Not related to your patch I think, but I think this would be nicer if the Timer name would be quoted.

> Timer "bar" already exists

This is better suited for other test case like multi-words timer name

> Timer "bar foo" already exists

Or if the name of the timer is "Timer"

> Timer "Timer" already exsits.

::: devtools/client/webconsole/new-console-output/test/fixtures/serviceContainer.js
@@ +12,5 @@
>    onViewSourceInScratchpad: () => {},
>    openNetworkPanel: () => {},
>    sourceMapService: {
>      subscribe: () => {},
> +    originalPositionFor: () => { return new Promise(resolve => {}) },

Is there a risk that this makes a test hang in the future since we're never resolving the Promise ?

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8865786 [details] [diff] [review]
console2.patch

Review of attachment 8865786 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/locales/en-US/webconsole.properties
@@ +94,5 @@
>  # string containing anchor doesn't matches to any property in the content.
>  Autocomplete.blank=  <- no result
>  
>  maxTimersExceeded=The maximum allowed number of timers in this page was exceeded.
>  timerAlreadyExists=Timer %S already exists.

Could we quote the Timer name here and in the one below ? See https://bugzilla.mozilla.org/show_bug.cgi?id=1363307#c4 for cases where it would help.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #4)
> Comment on attachment 8866048 [details] [diff] [review]
> tests-webconsole-time.patch
> 
> Review of attachment 8866048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One question about the fix for sourceMap and a comment about how we display
> timer name , but aside from that, everything looks good
> 
> :::
> devtools/client/webconsole/new-console-output/test/components/console-api-
> call.test.js
> @@ +146,5 @@
> > +    it("shows an error if called again", () => {
> > +      const message = stubPreparedMessages.get("timerAlreadyExists");
> > +      const wrapper = render(ConsoleApiCall({ message, serviceContainer }));
> > +
> > +      expect(wrapper.find(".message-body").text()).toBe("Timer bar already exists.");
> 
> Not related to your patch I think, but I think this would be nicer if the
> Timer name would be quoted.

OK, I'll update the patch once Andrea has final versions posted since I need to run the stub generation against what will land

> devtools/client/webconsole/new-console-output/test/fixtures/serviceContainer.
> js
> @@ +12,5 @@
> >    onViewSourceInScratchpad: () => {},
> >    openNetworkPanel: () => {},
> >    sourceMapService: {
> >      subscribe: () => {},
> > +    originalPositionFor: () => { return new Promise(resolve => {}) },
> 
> Is there a risk that this makes a test hang in the future since we're never
> resolving the Promise ?

Yeah, I guess we can go ahead and resolve right away

Comment 7

[Andrea Marchesini [:baku]]

Attachment: console2.patch

Comment 8

[Andrea Marchesini [:baku]]

Are you going to land these 3 patches all together?

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Andrea Marchesini [:baku] from comment #8)
> Are you going to land these 3 patches all together?

Yes, we may as well land them at the same time

Comment 10

[Brian Grinstead [:bgrins]]

Were you planning to upload new versions of the two patches?

Comment 11

[Andrea Marchesini [:baku]]

No. Those patches are OK for me.

Comment 12

[Brian Grinstead [:bgrins]]

Try push with all three patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85b29dd4b82837c3f17e0f11ebef3acb4bd5c80f

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fee9809299c
Console.timeEnd(label) should warn if 'label' in an unknwon timer, r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/91427ef3b016
Update component test for timer errors;r=nchevobbe

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9fee9809299c
https://hg.mozilla.org/mozilla-central/rev/91427ef3b016

Comment 15

[Md Maruf Rahman]

I have reproduced this bug with Nightly 55.0a1 (2017-05-09) (64-bit) on Windows 7, 64 Bit!

This bug's fix is verified with latest Nightly!

Build ID   : 20170517030204
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170517]

Comment 16

[Tanvir Rahman]

I have reproduced this bug with Nightly 55.0a1 (2017-05-09) on Ubuntu 16.04, 64 Bit!

The fix is now verified on latest Nightly!

Build ID 	20170523100217
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170524]

Comment 17

[Tanvir Rahman]

As per Comment 15 & Comment 16, I am marking this bug as verified fixed.