Bug 55482 - Web Inspector: introduce protocol test for RuntimeAgent.
Summary: Web Inspector: introduce protocol test for RuntimeAgent.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL:
Keywords:
Depends on: 55700
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-01 10:44 PST by Ilya Tikhonovsky
Modified: 2011-03-04 05:14 PST (History)
10 users (show)

See Also:


Attachments
[patch] initial version (8.38 KB, patch)
2011-03-01 10:48 PST, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
[patch] second iteration. (16.17 KB, patch)
2011-03-03 02:10 PST, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
[patch] third version (17.08 KB, patch)
2011-03-03 06:07 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] fourth version (16.72 KB, patch)
2011-03-03 07:20 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] fifth version (14.42 KB, patch)
2011-03-03 07:40 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2011-03-01 10:44:28 PST
%subj%
Comment 1 Ilya Tikhonovsky 2011-03-01 10:48:12 PST
Created attachment 84249 [details]
[patch] initial version
Comment 2 Pavel Feldman 2011-03-01 22:21:37 PST
Comment on attachment 84249 [details]
[patch] initial version

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

> LayoutTests/http/tests/inspector/inspector-test.js:226
> +InspectorTest.dumpStepResult = function(step, expression, result)

Please use InspectorTest.runTestSuite instead (look up for the syntax in other tests).

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:5
> +was called: "RuntimeAgent.evaluate('true', 'test', false, InspectorTest.callback)"

Given that this is a protocol test, it would be nice to dump both: protocol request and response:

{
    request : {
    }

    response : {
    }
}

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:8
> +    objectId : null

We should stop sending objectId and hasChildren for primitive values.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:19
> +    objectId : {

Form valid JSON (with "," - need to patch addObject).

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:20
> +        injectedScriptId : 1

This can become the source of flake. However, if it does not, lets leave it like this.
Btw, addObject allows printing types of members with given names instead of their values.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:21
> +        id : 1

This can become the source of flake

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:22
> +        groupName : "test"

I think we can safely remove groupName from objectId. We would need an id -> groupName map in injected script for getProperties that would be cleared whenever id is cleared.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:34
> +    $ : true

Two problems here:
1) it does not list getElementBy*
2) We should really send these in array of strings. That was we would be able to send "__proto__" suggestion. Should be a separate patch though.

Also, it is worth sending getCompletions('windo') and passing 'false' in includeCommandLineAPI.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:68
> +result = true

result should be success: true, right?

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:124
> +was called: "RuntimeAgent.getProperties(testObjectId, false, false, InspectorTest.callback)"

is this a dupe?
Comment 3 Ilya Tikhonovsky 2011-03-01 23:16:45 PST
(In reply to comment #2)

The idea of the protocol tests is to record the arguments which
were used and the format of answer which was receive for the each function.
The results produced by this test are showing us the sharp corners of current
implementation and the changes you've requested should be the target for other patches.

> (From update of attachment 84249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84249&action=review
> 
> > LayoutTests/http/tests/inspector/inspector-test.js:226
> > +InspectorTest.dumpStepResult = function(step, expression, result)
> 
> Please use InspectorTest.runTestSuite instead (look up for the syntax in other tests).

runTestSuite requires much more symbols for doing the same, very simple thing - just call each function and dump all the results.

> 
> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:5
> > +was called: "RuntimeAgent.evaluate('true', 'test', false, InspectorTest.callback)"
> 
> Given that this is a protocol test, it would be nice to dump both: protocol request and response:
> 
> {
>     request : {
>     }
> 
>     response : {
>     }
> }

Will do.

> 
> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:8
> > +    objectId : null
> 
> We should stop sending objectId and hasChildren for primitive values.

It should be the target for another patch.

> 
> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:19
> > +    objectId : {
> 
> Form valid JSON (with "," - need to patch addObject).

Will do.

> 
> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:20
> > +        injectedScriptId : 1
> 
> This can become the source of flake. However, if it does not, lets leave it like this.
> Btw, addObject allows printing types of members with given names instead of their values.
> 
> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:21
> > +        id : 1
> 
> This can become the source of flake
> 
> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:22
> > +        groupName : "test"
> 
> I think we can safely remove groupName from objectId. We would need an id -> groupName map in injected script for getProperties that would be cleared whenever id is cleared.

It should be the target for another patch.

> 
> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:34
> > +    $ : true
> 
> Two problems here:
> 1) it does not list getElementBy*
> 2) We should really send these in array of strings. That was we would be able to send "__proto__" suggestion. Should be a separate patch though.

It should be the target for another patch.

> 
> Also, it is worth sending getCompletions('windo') and passing 'false' in includeCommandLineAPI.

Will do.

> 
> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:68
> > +result = true
> 
> result should be success: true, right?

It should be the target for another patch.

> 
> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:124
> > +was called: "RuntimeAgent.getProperties(testObjectId, false, false, InspectorTest.callback)"
> 
> is this a dupe?

Nope. I'm checking that object property was assigned by the first setPropertyValue and dumped by the first getProperty was removed by the second setPropertyCall call.
Probably this is not a protocol related thing and should be moved to some functional test suite for RuntimeAgent.
Comment 4 Ilya Tikhonovsky 2011-03-03 02:10:13 PST
Created attachment 84538 [details]
[patch] second iteration.

> Given that this is a protocol test, it would be nice to dump both: protocol
> request and response:
> {
>    request : {
>    }
>    response : {
>    }
> }

done.

> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:20
> > +        injectedScriptId : 1
> 
> This can become the source of flake. However, if it does not, lets leave it
> like this.
> Btw, addObject allows printing types of members with given names instead of
> their values.
> 
> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:21
> > +        id : 1

done.


> > LayoutTests/inspector/protocol/runtime-agent-expected.txt:34
> > +    $ : true
> 
> Two problems here:
> 1) it does not list getElementBy*
> 2) We should really send these in array of strings. That was we would be able
> to send "__proto__" suggestion. Should be a separate patch though.
> 
> Also, it is worth sending getCompletions('windo') and passing 'false' in
> includeCommandLineAPI.

doesn't work as expected. No idea what is the reason.
Comment 5 Pavel Feldman 2011-03-03 02:50:34 PST
Comment on attachment 84538 [details]
[patch] second iteration.

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

> LayoutTests/http/tests/inspector/protocol-test.js:3
> +InspectorTest.filterProps = function(something, nondeterministicProps)

1. reuse addObject
2. do not use abbreviations (filterProperties)
3. rename something to a more informative name (object?)

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:6
> +sent:

request: {

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:14
> +        includeCommandLineAPI : false

It'd be better to default this to false.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:18
> +received:

response: {

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:21
> +    domain : "Runtime"

don't return domain

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:25
> +            objectId : null

don't return object ids for primitive values.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:28
> +            hasChildren : false

No need to send this property for primitive values.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:34
> +was called: "RuntimeAgent.evaluate("window","test",false)"

nuke "was called"

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:58
> +                groupName : "test"

No need to return groupName.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:62
> +            hasChildren : true

We should return ownPropertiesCound instead of this boolean flag. Rest can be determined on the caller site.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:68
> +was called: "RuntimeAgent.getCompletions("document.get",true)"

We should introduce evaluateAsJSON and use it instead.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:107
> +was called: "RuntimeAgent.getCompletions("window.testObje",true)"

Ditto.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:204
> +        result : true

There should be no output parameters.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:209
> +was called: "RuntimeAgent.setPropertyValue({"injectedScriptId":<number>,"id":<number>,"groupName":"test"},"removed_by_setPropertyValue","")"

There is a bug in implementation (weird catch)

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:249
> +            groupName : "test"

groupName:

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:332
> +was called: "RuntimeAgent.releaseWrapperObjectGroup(1,"test")"

Rename to releaseObjectGroup

> LayoutTests/inspector/protocol/runtime-agent.html:44
> +        , [ ["evaluate", 'true', 'test', false]

This all is too obfuscated and magical. Could we make the format more verbose so that others could understand / use it?

> LayoutTests/inspector/protocol/runtime-agent.html:45
> +          , ["evaluate", 'window', 'test', false]  

, should be after argument

> LayoutTests/inspector/protocol/runtime-agent.html:49
> +          , "InspectorTest.testObjectId = InspectorTest.callResult.objectId"

Is this a bug? All other members are arrays. We have not defined InspectorTest.callResult in this test, have we?

> LayoutTests/inspector/protocol/runtime-agent.html:56
> +        , {'injectedScriptId':true,'id':true}

Also a bug: all other members are arrays, we don't have "injectedScriptId" protocol method, do we?
Comment 6 Ilya Tikhonovsky 2011-03-03 06:07:10 PST
Created attachment 84549 [details]
[patch] third version

protocol test harness was adjusted.
Comment 7 Ilya Tikhonovsky 2011-03-03 07:20:43 PST
Created attachment 84558 [details]
[patch] fourth version
Comment 8 Ilya Tikhonovsky 2011-03-03 07:40:13 PST
Created attachment 84560 [details]
[patch] fifth version
Comment 9 Ilya Tikhonovsky 2011-03-03 12:23:21 PST
Comment on attachment 84560 [details]
[patch] fifth version

Clearing flags on attachment: 84560

Committed r80272: <http://trac.webkit.org/changeset/80272>
Comment 10 Ilya Tikhonovsky 2011-03-03 12:23:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Ilya Tikhonovsky 2011-03-04 05:14:48 PST
landed as r80341