Bug 44875 – Web Inspector: it'd be better to introduce inspector API related tests.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Ilya Tikhonovsky]

%subj%

[Ilya Tikhonovsky]

Created attachment 65930 [details]
[patch] initial version.

[Joseph Pecoraro]

Comment on attachment 65930 [details]
[patch] initial version.

> diff --git LayoutTests/ChangeLog
> [phone]  Ilya Tikhonovsky  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        WebInspector: As far as we have some kind of API for Inspector
> +        it'd be better to have API related tests. This is the test
> +        for API wrappers. These wrappers are tracking the types of arguments
> +        of API functions.

Missing  bug title + link. (Maybe this was expected if this was
webkit-patch upload?) Applies to all ChangeLogs in this patch.


> diff --git LayoutTests/inspector/report-API-errors.html
> +var test = function()
> +{
> +    var callback = function(message)
> +    {

This both use the "var x = function()" syntax. I find that unusual.
I think most of the other tests use the "function x()" syntax, and
I think that is easier to read. Tests typically have all kinds of
styles, so this isn't something to worry about, but the uncommon
syntax caught my eye.


> +    InspectorTest._addSniffer(WebInspector, "reportProtocolError", callback, true);
> +
> +    InspectorBackend.enableResourceTracking(1, callback);
> +    InspectorBackend.enableResourceTracking();
> +    InspectorBackend.enableResourceTracking(true, "not a function");


This covers "nice" messages with unusual arguments. This is very good.
Is there a plan for testing "bad" messages? Such as:

  InspectorFrontendHost.sendMessageToBackend("bad message")

I guess this shouldn't happen in practice, but if this really is
an over the wire protocol than those messages could be modified.
Testing that interface would be awesome too.


> diff --git WebCore/inspector/front-end/inspector.js
> -            console.error("Attempted to dispatch unimplemented WebInspector method: %s", messageObject.event);
> +            this.reportProtocolError("Attempted to dispatch unimplemented WebInspector method: %s", messageObject.event);

This changed an "sprintf" style console.error to a reportProtocolError function
call which does not respect sprintf style, and thus the 2nd argument is ignored.
I seem to recall an easy way to convert a function to the sprintf style, just
wrapping and replacing a function. Either way, could testing this error message
be added to your test?


> diff --git WebCore/inspector/front-end/inspector.js
> -WebInspector.reportProtocolError = function(messageObject)
> +WebInspector.reportProtocolError = function(message)
>  {
> -    console.error("Error: InspectorBackend request with seq = " + messageObject.seq + " failed.");
> +    if (typeof message === "string") {
> +        console.error("Protocol Error: " + message);
> +        return;
> +    }
> +
> +    console.error("Protocol Error: InspectorBackend request with seq = " + messageObject.seq + " failed.");
>      for (var error in messageObject.errors)
>          console.error("    " + error);
>      WebInspector.removeResponseCallbackEntry(messageObject.seq);

These last few lines are using "messageObject". However, there is no more
any "messageObject" in scope? are they needed any more? r- for this.

[Ilya Tikhonovsky]

Created attachment 65947 [details]
[patch] second iteration

(In reply to comment #2)
> (From update of attachment 65930 [details])
> > diff --git LayoutTests/ChangeLog
> > [phone]  Ilya Tikhonovsky  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        WebInspector: As far as we have some kind of API for Inspector
> > +        it'd be better to have API related tests. This is the test
> > +        for API wrappers. These wrappers are tracking the types of arguments
> > +        of API functions.
> 
> Missing  bug title + link. (Maybe this was expected if this was
> webkit-patch upload?) Applies to all ChangeLogs in this patch.

done.

> > diff --git LayoutTests/inspector/report-API-errors.html
> > +var test = function()
> > +{
> > +    var callback = function(message)
> > +    {
> 
> This both use the "var x = function()" syntax. I find that unusual.
> I think most of the other tests use the "function x()" syntax, and
> I think that is easier to read. Tests typically have all kinds of
> styles, so this isn't something to worry about, but the uncommon
> syntax caught my eye.

done


> > +    InspectorTest._addSniffer(WebInspector, "reportProtocolError", callback, true);
> > +
> > +    InspectorBackend.enableResourceTracking(1, callback);
> > +    InspectorBackend.enableResourceTracking();
> > +    InspectorBackend.enableResourceTracking(true, "not a function");
> 
> 
> This covers "nice" messages with unusual arguments. This is very good.
> Is there a plan for testing "bad" messages? Such as:
> 
>   InspectorFrontendHost.sendMessageToBackend("bad message")
> 
> I guess this shouldn't happen in practice, but if this really is
> an over the wire protocol than those messages could be modified.
> Testing that interface would be awesome too.

That will be the target of another test. In this test I'm trying to cover the functionality of
the internal Inspector JS API implemented in InspectorBackendStub.js

> > diff --git WebCore/inspector/front-end/inspector.js
> > -            console.error("Attempted to dispatch unimplemented WebInspector method: %s", messageObject.event);
> > +            this.reportProtocolError("Attempted to dispatch unimplemented WebInspector method: %s", messageObject.event);
> 
> This changed an "sprintf" style console.error to a reportProtocolError function
> call which does not respect sprintf style, and thus the 2nd argument is ignored.
> I seem to recall an easy way to convert a function to the sprintf style, just
> wrapping and replacing a function. Either way, could testing this error message
> be added to your test?

Just realized that I can sniff console.error instead of reportProtocolError.
New step for this use case was added and an error in the code was fixed. :)


> > diff --git WebCore/inspector/front-end/inspector.js
> > -WebInspector.reportProtocolError = function(messageObject)
> > +WebInspector.reportProtocolError = function(message)
> >  {
> > -    console.error("Error: InspectorBackend request with seq = " + messageObject.seq + " failed.");
> > +    if (typeof message === "string") {
> > +        console.error("Protocol Error: " + message);
> > +        return;
> > +    }
> > +
> > +    console.error("Protocol Error: InspectorBackend request with seq = " + messageObject.seq + " failed.");
> >      for (var error in messageObject.errors)
> >          console.error("    " + error);
> >      WebInspector.removeResponseCallbackEntry(messageObject.seq);
> 
> These last few lines are using "messageObject". However, there is no more
> any "messageObject" in scope? are they needed any more? r- for this.

This part of patch just removed.

[Ilya Tikhonovsky]

Created attachment 65950 [details]
[patch] third iteration.

unnecessary try catch was removed.

[Joseph Pecoraro]

Comment on attachment 65950 [details]
[patch] third iteration.

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog

I know I have talked about ChangeLog formatting before. It may
Be that I am too picky on this, but I find it much easier to read,
and ultimately make the entry more useful when well formatted.
the bug title and link to lead back to bugzilla for discussion
and important comments or reproducible steps. A separate paragraph
For details concerning the changes in the patch. prepare-ChangeLog
Provides this template.


> diff --git a/LayoutTests/inspector/report-API-errors-expected.txt
> +Protocol Error: Invalid type of argument 'always' for 'InspectorBackend.enableResourceTracking' call. It should be 'boolean' but it is 'number'.
> +Protocol Error: Invalid number of arguments for 'InspectorBackend.enableResourceTracking' call. It should have the next arguments '{"always":"boolean"}'.
> +Protocol Error: Optional callback argument for 'InspectorBackend.enableResourceTracking' call should be a function but its type is 'string'.
> +Protocol Error: Attempted to dispatch an unimplemented WebInspector method 'something-strange'

Bottom message doesn't have a period, but the others do.
I think it would be nice to be consistent here. For precedence
you could look at JavaScript syntax errors and if they print out
with a period. (I current cannot test this... iPad review!)


> +                console.error("Protocol Error: Invalid type of argument '%s' for 'InspectorBackend.%s' call. It should be '%s' but it is '%s'.", key, request.command, request.arguments[key], typeof(value));

> +                console.error("Protocol Error: Optional callback argument for 'InspectorBackend.%s' call should be a function but its type is '%s'.", request.command, typeof(args[0]));

We normally don't use typeof with ()s.


Patch looks much better. I will take a closer look tomorrow.

[Joseph Pecoraro]

Comment on attachment 65950 [details]
[patch] third iteration.

> ... you could look at JavaScript syntax errors and if they print out with a period.

JavaScript errors don't. So maybe these shouldn't. Thoughts?


> Patch looks much better. I will take a closer look tomorrow.

r=me addressing the comments I had above. Please file a follow-up, if there
isn't one already, for testing sendMessageToBackend message parsing and
dispatching.

[Joseph Pecoraro]

> Please file a follow-up, if there isn't one already...

Next email in my inbox! Thanks!
<http://webkit.org/b/44947> Web Inspector: The parser of Inspector protocol messages should be covered by a test.

[Ilya Tikhonovsky]

(In reply to comment #6)
> (From update of attachment 65950 [details])
> > ... you could look at JavaScript syntax errors and if they print out with a period.
> 
> JavaScript errors don't. So maybe these shouldn't. Thoughts?

As far as two error messages have period inside it'd be better to keep period at the end.

[Ilya Tikhonovsky]

r66515 = 501ce788f0c2beecbced5c5dde894f3f175e257c (refs/remotes/trunk)
	M	WebCore/ChangeLog
	M	WebCore/inspector/CodeGeneratorInspector.pm
	M	WebCore/inspector/front-end/inspector.js
	A	LayoutTests/inspector/report-API-errors-expected.txt
	A	LayoutTests/inspector/report-API-errors.html
	M	LayoutTests/ChangeLog
r66516 = 71b37b12202941ac9d982b7b421d90bf02ec88c6 (refs/remotes/trunk)