Summary: | WebInspector: RR for unit test of timeline data | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Ayers <zundel> | ||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, jaimeyap, joepeck, knorton, pfeldman, timothy, zundel | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Eric Ayers
2009-11-06 07:20:10 PST
Created attachment 42650 [details]
WebInspector: Adds a unit test for Timeline records
(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 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.
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. (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? (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. 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.)
(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. 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? 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.
Created attachment 42889 [details]
WebInspector: Adds a unit test for Timeline records
Fixes some formatting glitches.
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+. Created attachment 42960 [details]
WebInspector: Adds a unit test for Timeline records
Addresses review comments.
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> All reviewed patches have been landed. Closing bug. |