Web Inspector: add protocol test for existing error handling performed by the backend

• Component: Web Inspector
• Status: RESOLVED FIXED

[Brian Burg]

Prior to refactoring the error handling, I'd like to get some test coverage.

[Radar WebKit Bug Importer]

<rdar://problem/21892877>

[BJ Burg]

Created attachment 259790 [details]
Proposed Fix

[Joseph Pecoraro]

Comment on attachment 259790 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=259790&action=review

Nice!

> LayoutTests/inspector/protocol/backend-dispatcher-argument-errors.html:71
> +        name : "RequiredParameterWrongType",
> +        description: "The backend should return an error if a message has an optional parameter with wrong type.",

The name here should be "OptionalParameterWrongType".

> LayoutTests/inspector/protocol/backend-dispatcher-malformed-message-errors.html:17
> +    let errorCodes = {
> +        ParseError: -32700,
> +        InvalidRequest: -32600,
> +        MethodNotFound: -32601,
> +        InvalidParams: -32602,
> +        InternalError: -32603,
> +        ServerError: -32000,
> +    };

Should InspectorBackend expose these? Currently it only checks for -32000 but it could expose these constants. Then we wouldn't need to duplicate it in these tests.

> LayoutTests/inspector/protocol/backend-dispatcher-malformed-message-errors.html:39
> +                })

Style: semicolon? I'm normally very lax about commenting on style of tests, but seeing a they are getting rather clean / uniform now.

> LayoutTests/inspector/protocol/backend-dispatcher-malformed-message-errors.html:46
> +        name : "UnparseableStringMessage",

Style: "name :" => "name:" (for all of these in both tests)

> LayoutTests/inspector/protocol/backend-dispatcher-malformed-message-errors.html:141
> +    addErrorResponseTestCase({
> +        name : "MethodFieldWithBadFormatting5",
> +        description: "The backend should return an error if a message has a 'method' field not formatted as 'Domain.Methodname'.",
> +        message: {id: 123, method: ".FooBar."},
> +        expectedError: "InvalidRequest"
> +    });

Another similar one would be Foo.bar.baz. Both covered by the same condition here, but might be good to test separately.

[BJ Burg]

Comment on attachment 259790 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=259790&action=review

>> LayoutTests/inspector/protocol/backend-dispatcher-malformed-message-errors.html:17
>> +    };
> 
> Should InspectorBackend expose these? Currently it only checks for -32000 but it could expose these constants. Then we wouldn't need to duplicate it in these tests.

Meh, they are part of the JSON-RPC specification.

Besides, this test doesn't cover InspetorBackend.js, which should be a separate test (focused on correct resolve/reject/callback args).

>> LayoutTests/inspector/protocol/backend-dispatcher-malformed-message-errors.html:39
>> +                })
> 
> Style: semicolon? I'm normally very lax about commenting on style of tests, but seeing a they are getting rather clean / uniform now.

It's a promise chain.

[BJ Burg]

Committed r188897: <http://trac.webkit.org/changeset/188897>