Bug 55306 - Web Inspector: add first network test, improve harness.
Summary: Web Inspector: add first network test, improve harness.
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2011-02-26 14:19 PST by Pavel Feldman
Modified: 2011-02-28 22:10 PST (History)
11 users (show)

See Also:

Patch (11.47 KB, patch)
2011-02-27 23:11 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (11.50 KB, patch)
2011-02-28 03:14 PST, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-02-26 14:19:39 PST
Patch to follow.
Comment 1 Pavel Feldman 2011-02-27 23:11:59 PST
Created attachment 84021 [details]
Comment 2 Yury Semikhatsky 2011-02-27 23:57:58 PST
Comment on attachment 84021 [details]

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

> LayoutTests/http/tests/inspector/inspector-test.js:308
>      layoutTestController.closeWebInspector();

Please delete watchdog timer here to make sure we don't close inspector twice.

> LayoutTests/http/tests/inspector/network/network-size.html:33
> +        var resource1 = WebInspector.panels.network.resources[0];

Image order might change. Can you sort the results to avoid inconsistent output? r- for this.

> LayoutTests/http/tests/inspector/network/network-timing.html:34
> +        InspectorTest.addResult(resource1.url);

Results' order should not depend on network activity.

> LayoutTests/http/tests/inspector/network/network-timing.html:36
> +        InspectorTest.assertGreaterOrEqual(300, resource1.duration * 1000, "Duration of the first resource");

I don't like that we deliberately slow down layout tests. Can we use mocks/something else to avoid this. Inspector tests are already notorious for being quite slow.

> LayoutTests/http/tests/inspector/network/network-timing.html:41
> +        InspectorTest.assertGreaterOrEqual(100, resource2.latency * 1000, "Latency of the second resource");

I think we should print something in case of success.

> LayoutTests/http/tests/inspector/network/resources/resource.php:42
> +    var now = new Date();

Just use Date.now(). This way you won't need Number(now) cast below.

> LayoutTests/http/tests/inspector/network/resources/resource.php:55
> +            echo("function foo() {}");

Could be rewritten as
if (!$jscontent)
    $jscontent = "function foo() {}";
and use same logic for both branches.
Comment 3 Pavel Feldman 2011-02-28 03:14:13 PST
Created attachment 84037 [details]
Comment 4 Pavel Feldman 2011-02-28 03:21:53 PST
Committed r79849: <http://trac.webkit.org/changeset/79849>
Comment 5 James Robinson 2011-02-28 18:56:11 PST
This watchdog is evil, evil, evil, evil!  It prevents us from using slow/timeout/etc expectations as we want.

You should _never_ need to do your own watchdog timer for layout tests, the layout test harness has a timeout per test that we can modify as needed (for example SLOW modifiers, debug/release tweaks, TIMEOUT expectations).  Can you please remove this?  Otherwise we'll have to =TEXT a lot of slow-running inspector tests - which is a whole lot of them in debug!
Comment 6 Pavel Feldman 2011-02-28 22:10:08 PST
(In reply to comment #5)
> This watchdog is evil, evil, evil, evil!  It prevents us from using slow/timeout/etc expectations as we want.

Ok, ok. We need this for debugging mostly because we are special. Inspector tests are happening in the front-end mostly, and when there is a problem, we end up timed out. In that case, not all the outputs are flushed into the page, so I added a watchdog doing that. I did not think of SLOW.

Let me follow up and make sure watchdog is flushing, but not notifying done. As a result we all will be happy.