Bug 54729 - Web Inspector: flakyness of inspector tests
Summary: Web Inspector: flakyness of inspector tests
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:
Blocks:
 
Reported: 2011-02-18 00:33 PST by Ilya Tikhonovsky
Modified: 2011-02-22 07:30 PST (History)
10 users (show)

See Also:


Attachments
[patch] initial version (3.47 KB, patch)
2011-02-18 01:01 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] initial version (26.30 KB, patch)
2011-02-22 02:55 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] third version (25.96 KB, patch)
2011-02-22 05:04 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
the patch I'm going to land. (22.96 KB, patch)
2011-02-22 07:30 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-02-18 00:33:39 PST
many inspector tests are flaky.

I think the reason for this is a race between two pages.
We have inspected page and Inspector's frontend page. 
When frontend page is loaded it begins to interact with
Inspector's backend to gather information about the state
of the inspecting page. This process is completely asynchronous. 
A number of messages will be sent back and forth. 
At the same time the inspected page tries do its work and
send their messages to check the state inspector and trying
to do some things with it. However, the state of Inspector's page
is not stable until it finishes its initialization process. 

it would be better to wait until Inspector's frontend page will send
all their requests and receive back all the answers.
We definitely know how many requests are waiting for answers and need
just wait until the number becomes 0 and process test command after that.
Comment 1 Ilya Tikhonovsky 2011-02-18 01:01:59 PST
Created attachment 82931 [details]
[patch] initial version
Comment 2 Alexander Pavlov (apavlov) 2011-02-18 02:06:33 PST
Comment on attachment 82931 [details]
[patch] initial version

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

> Source/WebCore/ChangeLog:6
> +

You should specify a bug URL after the issue title

> Source/WebCore/inspector/CodeGeneratorInspector.pm:629
> +        return !!this._waitForResponse;

The boolean cast is not necessary, and we normally do not use it (according to one of pfeldman's reviews)
Comment 3 Pavel Feldman 2011-02-18 02:50:43 PST
Comment on attachment 82931 [details]
[patch] initial version

We already have a counter for runAfterPendingDispatches.
Comment 4 Ilya Tikhonovsky 2011-02-22 02:55:14 PST
Created attachment 83293 [details]
[patch] initial version

now test harness:
1) doesn't use setTimeout for processing pending test scripts;
2) has no difference between WebKit and Chromium implementations;
3) eval pending test scripts only when all the responses are received.
Comment 5 Pavel Feldman 2011-02-22 03:05:25 PST
Comment on attachment 83293 [details]
[patch] initial version

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

> Source/WebCore/inspector/CodeGeneratorInspector.pm:714
> +    this._waitForResponse = 0;

this._pendingResponseCount

> Source/WebCore/inspector/CodeGeneratorInspector.pm:732
> +        --this._waitForResponse;

move this line to after callback.apply

> Source/WebCore/inspector/CodeGeneratorInspector.pm:815
> +            if (this.runAfterPendingDispatches && WebInspector.pendingDispatches === 0 && !this.isWaitingForResponse())

pendingDispatches is of no use now.

> Source/WebCore/inspector/front-end/TestController.js:78
> +WebInspector.testController = new WebInspector.TestController();

You should not need it (leave as it was).

> Source/WebCore/inspector/front-end/TestController.js:79
> +WebInspector.evaluateForTestInFrontend = WebInspector.testController._evaluateForTestInFrontend.bind(WebInspector.testController);

Leave as it was.
Comment 6 Ilya Tikhonovsky 2011-02-22 05:04:08 PST
Created attachment 83301 [details]
[patch] third version

comments addressed
Comment 7 Pavel Feldman 2011-02-22 05:28:55 PST
Comment on attachment 83301 [details]
[patch] third version

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

> Source/WebCore/inspector/CodeGeneratorInspector.pm:733
> +        --this._pendingResponsesCount;

you should move this into dispatch (after the if/else).

> Source/WebCore/inspector/CodeGeneratorInspector.pm:815
> +            if (this.runAfterPendingDispatches && !this.isWaitingForResponse())

Just move it here.

> Source/WebCore/inspector/front-end/TestController.js:77
> +    WebInspector.testController.runAfterPendingDispatches(invokeMethod);

I'd rather implement runAfterPendingDispatches on the InspectorBackendStub and use it both: from test controller and inspectortest.
Comment 8 Ilya Tikhonovsky 2011-02-22 07:28:56 PST
Committed r79322
	M	LayoutTests/http/tests/inspector/inspector-test.js
	M	LayoutTests/ChangeLog
	M	Source/WebKit/chromium/ChangeLog
	M	Source/WebKit/chromium/src/js/DevTools.js
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/inspector/CodeGeneratorInspector.pm
	M	Source/WebCore/inspector/front-end/inspector.js
	M	Source/WebCore/inspector/front-end/TestController.js
	D	Tools/DumpRenderTree/chromium/DRTDevToolsCallArgs.cpp
	D	Tools/DumpRenderTree/chromium/DRTDevToolsCallArgs.h
	M	Tools/DumpRenderTree/chromium/DRTDevToolsClient.cpp
	M	Tools/DumpRenderTree/chromium/DRTDevToolsAgent.h
	M	Tools/DumpRenderTree/chromium/DRTDevToolsClient.h
	M	Tools/DumpRenderTree/chromium/DRTDevToolsAgent.cpp
	M	Tools/DumpRenderTree/DumpRenderTree.gypi
	M	Tools/ChangeLog
W: -empty_dir: trunk/Tools/DumpRenderTree/chromium/DRTDevToolsCallArgs.cpp
W: -empty_dir: trunk/Tools/DumpRenderTree/chromium/DRTDevToolsCallArgs.h
r79322 = 169c0aa5e9b5d1327052690447638f5bcfa812cc (refs/remotes/trunk)
Comment 9 Ilya Tikhonovsky 2011-02-22 07:30:37 PST
Created attachment 83311 [details]
the patch I'm going to land.