Bug 55816 - Web Inspector: fix layout tests flakiness.
Summary: Web Inspector: fix layout tests flakiness.
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-04 23:23 PST by Pavel Feldman
Modified: 2011-03-05 02:09 PST (History)
10 users (show)

See Also:


Attachments
Patch (33.51 KB, patch)
2011-03-04 23:28 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
[PATCH] Same with chromium baselines. (38.20 KB, patch)
2011-03-04 23:59 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
[PATCH] Same with everything passing in debug. (40.32 KB, patch)
2011-03-05 01:48 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (40.94 KB, patch)
2011-03-05 01:57 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-03-04 23:23:31 PST
A number of fixes were applied, there were multiple sources of flakiness.
- Order of issuing of evaluateForTestInFrontend was not guaranteed on the backend side (InspectorAgent side)
- Order of dispatching using timeouts was guaranteed via queueing. Source of all kinds of pains on SnowLeopard Release (inspector.js)
- Timeline now uses instrumentation calls, not UI representation while collecting events for tests
- No reloads in debugger tests that don't need them
- Forcing layout in timeline via calculating offsetHeight
Comment 1 Pavel Feldman 2011-03-04 23:28:19 PST
Created attachment 84846 [details]
Patch
Comment 2 Pavel Feldman 2011-03-04 23:59:13 PST
Created attachment 84849 [details]
[PATCH] Same with chromium baselines.
Comment 3 Yury Semikhatsky 2011-03-05 00:01:05 PST
Comment on attachment 84846 [details]
Patch

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

> LayoutTests/http/tests/inspector/inspector-test.js:331
> +        var intermediate = document.createElement("div");

Why do you need this?

> LayoutTests/inspector/console/console-uncaught-exception-expected.txt:6
> +console-uncaught-exception.html:40Error: Exception in inline script.

Chromium has stack traces for uncaught exceptions when front-end is open, this is why we need to reload inspected page. I don't see changes to the chromium-specific expectations in this patch. r- for this.
Comment 4 Yury Semikhatsky 2011-03-05 00:07:58 PST
Comment on attachment 84849 [details]
[PATCH] Same with chromium baselines.

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

> LayoutTests/platform/chromium/inspector/console/console-uncaught-exception-expected.txt:6
> +console-uncaught-exception.html:40Uncaught Error: Exception in inline script.aconsole-uncaught-exception.html:40bconsole-uncaught-exception.html:45(anonymous function)console-uncaught-exception.html:48

How can you be sure that frontend is already loaded when the exceptions are thrown?

> Source/WebCore/inspector/InspectorAgent.cpp:399
> +    m_pendingEvaluateTestCommands.clear();

Why do we need this magic?
Comment 5 Pavel Feldman 2011-03-05 00:33:23 PST
(In reply to comment #3)
> (From update of attachment 84846 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84846&action=review
> 
> > LayoutTests/http/tests/inspector/inspector-test.js:331
> > +        var intermediate = document.createElement("div");
> 
> Why do you need this?
> 

So that output was not the top-level element and was not generating DOM events upon every addResult invocation.

> > LayoutTests/inspector/console/console-uncaught-exception-expected.txt:6
> > +console-uncaught-exception.html:40Error: Exception in inline script.
> 
> Chromium has stack traces for uncaught exceptions when front-end is open, this is why we need to reload inspected page. I don't see changes to the chromium-specific expectations in this patch. r- for this.

Uploading patch with proper expectations and flow.


> How can you be sure that frontend is already loaded when the exceptions are thrown?
> 

See above.

> > Source/WebCore/inspector/InspectorAgent.cpp:399
> > +    m_pendingEvaluateTestCommands.clear();
> 
> Why do we need this magic?

When single inspector agent is reused, we'd like to clear potential pending messages so that they don't get fired with the next front-end. This is a paranoid check.
Comment 6 Pavel Feldman 2011-03-05 01:48:32 PST
Created attachment 84852 [details]
[PATCH] Same with everything passing in debug.
Comment 7 Yury Semikhatsky 2011-03-05 01:55:38 PST
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 84846 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=84846&action=review
> > 
> > > LayoutTests/http/tests/inspector/inspector-test.js:331
> > > +        var intermediate = document.createElement("div");
> > 
> > Why do you need this?
> > 
> 
> So that output was not the top-level element and was not generating DOM events upon every addResult invocation.

A comment in the code would be very helpful for people who will read this code later.
Comment 8 Pavel Feldman 2011-03-05 01:57:27 PST
Created attachment 84853 [details]
Patch
Comment 9 Pavel Feldman 2011-03-05 02:09:58 PST
Committed r80416: <http://trac.webkit.org/changeset/80416>