Bug 31204 – WebInspector: RR for unit test of timeline data

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Eric Ayers]

This is more just to show a work in progress and show where I am going with the LayoutTests for the Timeline data

There has not yet been an attempt to use Pavel's recently updated modular test infrastructure.   I've been keeping all the test infrastructure in timeline-test-api.js

Currently, the test turns on the timeline, reloads the page, then after the page is loaded the second time, dumps all the inspector console messages and timeline data.

The timeline data is scrubbed of events that appear to be related to the test framework infrastructure by marking nodes with the console.markTimeline API.
The timeline data is also scrubbed of the start/end times which will cause the layout test to fail 

I'm concerned that a test that looks like this will be too brittle and difficult to tell when it needs updating, so I'm considering an even more abbreviated format.  The new format would put each event on a different line, indented with the level of nesting in order to make a diff come out looking nice when the test fails.

Current:

{"children":[{"data":{"url":"file:///Users/zundel/apu-webkit/WebKit/LayoutTests/inspector/timeline-test-api.js","lineNumber":1},"children":[],"type":10},{"data":{"type":"load"},"children":[],"type":0},{"data":{"url":"file:///Users/zundel/apu-webkit/WebKit/LayoutTests/inspector/timeline-script-tag-1.html?reload","lineNumber":19},"children":[],"type":10}],"type":4}

would become:

ParseHTML
    EvaluateScript
    EventDispatch
    EvaluateScript

If we wanted more details on a particular event, maybe we could turn them on for one test and get a record like this

timelineExpandEvent(TimelineAgent.RecordType.EventDispatch);

ParseHTML
    EvaluateScript
    EventDispatch :  {"data":{"type":"load"},"type":0}
    EvaluateScript


That way, we can write a test that is sensitive to the change to one event type without causing all the timeline tests to fail.

[Eric Ayers]

Created attachment 42650 [details]
WebInspector: Adds a unit test for Timeline records

[Eric Ayers]

(Here's an updated test output using the simplified tree)

ResourceSendRequest
ResourceReceiveResponse
RecalculateStyles
RecalculateStyles
ResourceSendRequest
ResourceReceiveResponse
ResourceFinish
ParseHTML
----> EvaluateScript
----> EventDispatch
----> EvaluateScript
ResourceFinish
Layout
----> Layout
ParseHTML
EventDispatch
EventDispatch
----> TimerInstall
Paint
Layout

[Eric Ayers]

Created attachment 42667 [details]
WebInspector: Adds a unit test for Timeline records

I am trying to minimize the amount of data output by the test so that it won't be brittle.  I could go further by not printing out the ParseHTML timeline record.

[Pavel Feldman]

Comment on attachment 42667 [details]
WebInspector: Adds a unit test for Timeline records

> -    setTimeout(function() {
> -        if (window.layoutTestController)
> -            layoutTestController.showWebInspector();
> -        window.location.href += "?reload";
> -    }, 0);
> +    if (window.layoutTestController)
> +        layoutTestController.showWebInspector();

Sync opening of web inspector will fail on windows as far as i remember.

> +            }, 100); // give other scripts a chance to do work before the reload

100ms is not going to work on bots. What 'other' scripts and 'work' do you refer to here?

> -    }
> -    evaluateInWebInspector(toInject.join("\n"), doit);
> +    doit();

It is important that doit is called after evaluateInWebInspector invoked its callback.

> +if (window.layoutTestController) {
> +    layoutTestController.dumpAsText();
> +    layoutTestController.waitUntilDone();
> +}

You have it in inspector-test.js already.

> +        }
> +        markCurrentRecordAsOverhead("onload" + (ignoreLoad ? ":ignoreLoad": ""));
> +    }, 1000);  // Give timer fires created in script a chance to run.  Is 1 second too long to delay?
> +}

Again, time won't help. What is the goal here?

> +// We ignore initial load of the page, enable inspector and initiate reload. 
> +// This allows inspector controller to capture events that happen during the 
> +// initial page load, and startup the Timeline to capture all the records from
> +// loading the second page.

I'd like to keep reload logic in one place. We can extend inspector-test to make it call into
frontend_setUp early in the cycle.

[Eric Ayers]

(In reply to comment #4)
> (From update of attachment 42667 [details])
> > -    setTimeout(function() {
> > -        if (window.layoutTestController)
> > -            layoutTestController.showWebInspector();
> > -        window.location.href += "?reload";
> > -    }, 0);
> > +    if (window.layoutTestController)
> > +        layoutTestController.showWebInspector();
> 
> Sync opening of web inspector will fail on windows as far as i remember.
> 
> > +            }, 100); // give other scripts a chance to do work before the reload
> 
> 100ms is not going to work on bots. What 'other' scripts and 'work' do you
> refer to here?

The idea was not for 100ms, but to get behind other setTimeout(xxx,0) methods I had setup in timline-test.js.

> 
> > -    }
> > -    evaluateInWebInspector(toInject.join("\n"), doit);
> > +    doit();
> 
> It is important that doit is called after evaluateInWebInspector invoked its
> callback.

I modified the code so that the scripts are injected before the reload.   The functions should have long been defined before we get to this point, or is it that you want to defer execution of doit() until all of onload() has finished?

> 
> > +if (window.layoutTestController) {
> > +    layoutTestController.dumpAsText();
> > +    layoutTestController.waitUntilDone();
> > +}
> 
> You have it in inspector-test.js already.

Removed.

> 
> > +        }
> > +        markCurrentRecordAsOverhead("onload" + (ignoreLoad ? ":ignoreLoad": ""));
> > +    }, 1000);  // Give timer fires created in script a chance to run.  Is 1 second too long to delay?
> > +}
> 
> Again, time won't help. What is the goal here?

When I write a test script that does a timer fire, I want to make sure the timer fire goes off before we try to pull down the timeline data.  I want to queue up the analysis after all the other script activity we might perform, like a timer fire or XHR.  I know testing XHR is going to be problematic.  What do you suggest I do?
 
> > +// We ignore initial load of the page, enable inspector and initiate reload. 
> > +// This allows inspector controller to capture events that happen during the 
> > +// initial page load, and startup the Timeline to capture all the records from
> > +// loading the second page.
> 
> I'd like to keep reload logic in one place. We can extend inspector-test to
> make it call into
> frontend_setUp early in the cycle.

Why don't we have a prefix like frontendSetup_ and inject and execute all of those methods before reload?

[Pavel Feldman]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 42667 [details] [details])
> > > -    setTimeout(function() {
> > > -        if (window.layoutTestController)
> > > -            layoutTestController.showWebInspector();
> > > -        window.location.href += "?reload";
> > > -    }, 0);
> > > +    if (window.layoutTestController)
> > > +        layoutTestController.showWebInspector();
> > 
> > Sync opening of web inspector will fail on windows as far as i remember.
> > 
> > > +            }, 100); // give other scripts a chance to do work before the reload
> > 
> > 100ms is not going to work on bots. What 'other' scripts and 'work' do you
> > refer to here?
> 
> The idea was not for 100ms, but to get behind other setTimeout(xxx,0) methods I
> had setup in timline-test.js.
> 

setTimeout(xxx, 0) just queues them. No need in numbers.

> > 
> > > -    }
> > > -    evaluateInWebInspector(toInject.join("\n"), doit);
> > > +    doit();
> > 
> > It is important that doit is called after evaluateInWebInspector invoked its
> > callback.
> 
> I modified the code so that the scripts are injected before the reload.   The
> functions should have long been defined before we get to this point, or is it
> that you want to defer execution of doit() until all of onload() has finished?

When page loads, it pushes dom into the frontend. The main idea is that test execution continues (and does doit) when frontend received that information, made additional async calls, received response and everything settles down. That's what you violate here.

> 
> > 
> > > +if (window.layoutTestController) {
> > > +    layoutTestController.dumpAsText();
> > > +    layoutTestController.waitUntilDone();
> > > +}
> > 
> > You have it in inspector-test.js already.
> 
> Removed.
> 
> > 
> > > +        }
> > > +        markCurrentRecordAsOverhead("onload" + (ignoreLoad ? ":ignoreLoad": ""));
> > > +    }, 1000);  // Give timer fires created in script a chance to run.  Is 1 second too long to delay?
> > > +}
> > 
> > Again, time won't help. What is the goal here?
> 
> When I write a test script that does a timer fire, I want to make sure the
> timer fire goes off before we try to pull down the timeline data.  I want to
> queue up the analysis after all the other script activity we might perform,
> like a timer fire or XHR.  I know testing XHR is going to be problematic.  What
> do you suggest I do?
> 

For timers, setTimeout(0) would queue the fires. Isn't it enough?


> > > +// We ignore initial load of the page, enable inspector and initiate reload. 
> > > +// This allows inspector controller to capture events that happen during the 
> > > +// initial page load, and startup the Timeline to capture all the records from
> > > +// loading the second page.
> > 
> > I'd like to keep reload logic in one place. We can extend inspector-test to
> > make it call into
> > frontend_setUp early in the cycle.
> 
> Why don't we have a prefix like frontendSetup_ and inject and execute all of
> those methods before reload?

Fine with me, but you would need to care about order. While explicit frontend_setup sounds inline with unit testing in general.

[Eric Ayers]

Created attachment 42670 [details]
WebInspector: Adds a unit test for Timeline records

Incorporates review feedback, adding the optional execution of frontend_setup() method before the reload is executed.
All timers are set with 0 interval.

all tests in the inspector directory pass, (but the --interval 100 hasn't finshed yet.)

[Eric Ayers]

(In reply to comment #6)
...
> > 
> > The idea was not for 100ms, but to get behind other setTimeout(xxx,0) methods I
> > had setup in timline-test.js.
> > 
> 
> setTimeout(xxx, 0) just queues them. No need in numbers.

changed all timeouts to 0.

> > > > -    evaluateInWebInspector(toInject.join("\n"), doit);
> > > > +    doit();
> > > 
> > > It is important that doit is called after evaluateInWebInspector invoked its
> > > callback.
> > 
> > I modified the code so that the scripts are injected before the reload.   The
> > functions should have long been defined before we get to this point, or is it
> > that you want to defer execution of doit() until all of onload() has finished?
> 
> When page loads, it pushes dom into the frontend. The main idea is that test
> execution continues (and does doit) when frontend received that information,
> made additional async calls, received response and everything settles down.
> That's what you violate here.

added an executeInWebInspector() with a trivial body to invoke doit

> > > > +        }
> > > > +        markCurrentRecordAsOverhead("onload" + (ignoreLoad ? ":ignoreLoad": ""));
> > > > +    }, 1000);  // Give timer fires created in script a chance to run.  Is 1 second too long to delay?
> > > > +}
> > > 
> > > Again, time won't help. What is the goal here?
> > 
> > When I write a test script that does a timer fire, I want to make sure the
> > timer fire goes off before we try to pull down the timeline data.  I want to
> > queue up the analysis after all the other script activity we might perform,
> > like a timer fire or XHR.  I know testing XHR is going to be problematic.  What
> > do you suggest I do?
> > 
> 
> For timers, setTimeout(0) would queue the fires. Isn't it enough?

Now, the analysis occurs when you ask for it with retrieveTimelineData(). If you ask for it in doit() it will run as soon as possible.


> > > I'd like to keep reload logic in one place. We can extend inspector-test to
> > > make it call into
> > > frontend_setUp early in the cycle.
> > 
> > Why don't we have a prefix like frontendSetup_ and inject and execute all of
> > those methods before reload?
> 
> Fine with me, but you would need to care about order. While explicit
> frontend_setup sounds inline with unit testing in general.

inspector-test.js now injects the functions before reload, and executes frontend_setup() if it exists.

[Pavel Feldman]

Comment on attachment 42670 [details]
WebInspector: Adds a unit test for Timeline records

> +    // Synchronous opening of web inspector may fail on Windows

Wait a minute, we don't want that - we do run these on windows bots and need tests to succeed. You'll need to put stuff under onload / ignoreLoad into setTimeout to queue I guess once this one is reverted.


> +        // Invoke a setup method if it has been specified
> +        toInject.push("frontend_setup ? frontend_setup() : undefined");

"(if (frontend_setup) frontend_setup();)" ?



General comments: 
1. { } are used for single-line ifs all over the place
2. Use === instead of == for primitive types

Otherwise looks good.

Why added svn:executable flag?

[Eric Ayers]

Created attachment 42798 [details]
WebInspector: Adds a unit test for Timeline records

Added a few changes since the last patch:

- Addressed braces style comment
- Added 'timeline' to name of all functions meant to be used in tests
- Changed the output of the test to only print the MarkTImeline node and its ancestors.  Intended to make the test less brittle to changes not related to the script tag
- Changed the output of the test to print the properties of the EvaluateScript timeline record.  If there is a new property added or one removed, the test will need to be updated.

Tested this patch with --iterations 100 on all scripts in the LayoutTests/inspetor directory.

[Eric Ayers]

Created attachment 42889 [details]
WebInspector: Adds a unit test for Timeline records

Fixes some formatting glitches.

[Pavel Feldman]

Comment on attachment 42889 [details]
WebInspector: Adds a unit test for Timeline records

> +        var invokeSetup = false;

Is this unused?

> +}
> \ No newline at end of file

Please add newline.

Otherwise r+.

[Eric Ayers]

Created attachment 42960 [details]
WebInspector: Adds a unit test for Timeline records

Addresses review comments.

[WebKit Commit Bot]

Comment on attachment 42960 [details]
WebInspector: Adds a unit test for Timeline records

Clearing flags on attachment: 42960

Committed r50809: <http://trac.webkit.org/changeset/50809>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.