Tests for WebUI Hung Renderer Dialog

Tests for WebUI Hung Renderer Dialog

This CL adds a WebUIBrowserTest to test the WebUI Hung Renderer Dialog.

BUG=102073
TEST=browser_tests HungRendererDialogUITest.testHungRenderer


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108924

[wyck, 2011-11-04 20:15:24]

Note: This doesn't include an integration test for killing a tab, because scr@
and I are still working out how to do that.  But some other basic tests are
there.

[flackr, 2011-11-05 13:19:39]

http://codereview.chromium.org/8372042/diff/6001/chrome/test/data/webui/hung_...
File chrome/test/data/webui/hung_renderer_dialog_test.js (right):

http://codereview.chromium.org/8372042/diff/6001/chrome/test/data/webui/hung_...
chrome/test/data/webui/hung_renderer_dialog_test.js:53:
assertGT(themeGraphic.width, 0);
Use expect for checks that aren't fatal.

http://codereview.chromium.org/8372042/diff/6001/chrome/test/data/webui/hung_...
chrome/test/data/webui/hung_renderer_dialog_test.js:54:
assertGT(themeGraphic.height, 0);
I think these themeGraphic checks should be in a separate test. Their success /
failure doesn't need to affect the rest of the tests here.

[wyck, 2011-11-07 20:39:24]

Split out the tests anyway as per your wisdom.

http://codereview.chromium.org/8372042/diff/6001/chrome/test/data/webui/hung_...
File chrome/test/data/webui/hung_renderer_dialog_test.js (right):

http://codereview.chromium.org/8372042/diff/6001/chrome/test/data/webui/hung_...
chrome/test/data/webui/hung_renderer_dialog_test.js:54:
assertGT(themeGraphic.height, 0);
On 2011/11/05 13:19:40, flackr wrote:
> I think these themeGraphic checks should be in a separate test. Their success
/
> failure doesn't need to affect the rest of the tests here.
My thinking is that BrowserTests are expensive to run.  ~1300 ms.  So it would
be better to have the tests performed in the same run.  Since the individual
assertions are read-only operations (I don't invoke anything special to change
the DOM or run any special javascript functions) I thought it was more
appropriate to have them in the same test.

[flackr, 2011-11-07 21:00:13]

LGTM

http://codereview.chromium.org/8372042/diff/6001/chrome/test/data/webui/hung_...
File chrome/test/data/webui/hung_renderer_dialog_test.js (right):

http://codereview.chromium.org/8372042/diff/6001/chrome/test/data/webui/hung_...
chrome/test/data/webui/hung_renderer_dialog_test.js:54:
assertGT(themeGraphic.height, 0);
On 2011/11/07 20:39:24, wyck wrote:
> On 2011/11/05 13:19:40, flackr wrote:
> > I think these themeGraphic checks should be in a separate test. Their
success
> /
> > failure doesn't need to affect the rest of the tests here.
> My thinking is that BrowserTests are expensive to run.  ~1300 ms.  So it would
> be better to have the tests performed in the same run.  Since the individual
> assertions are read-only operations (I don't invoke anything special to change
> the DOM or run any special javascript functions) I thought it was more
> appropriate to have them in the same test.

I understand your point, however with both testable items containing assertions
a failure in one would have prevented testing the other even though they were
mutually exclusive. I think while it will take longer to run, it will provide
better feedback and test coverage in the event that one or the other test
breaks, and ultimately reduce time spent debugging failed tests (hopefully). I
may be wrong though.

http://codereview.chromium.org/8372042/diff/12001/chrome/test/data/webui/hung...
File chrome/test/data/webui/hung_renderer_dialog_test.js (right):

http://codereview.chromium.org/8372042/diff/12001/chrome/test/data/webui/hung...
chrome/test/data/webui/hung_renderer_dialog_test.js:26: function
HungRendererDialogUITestAsync() {};
This seems to be unused, remove please.

[commit-bot: I haz the power, 2011-11-07 21:01:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wyck@chromium.org/8372042/12001

[commit-bot: I haz the power, 2011-11-07 21:08:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wyck@chromium.org/8372042/13006

[commit-bot: I haz the power, 2011-11-07 22:38:42]

Change committed as 108924

[wyck, 2011-11-10 06:45:56]

I fixed the issue that caused the revert.  On WinXP OnDialogClose is called for
a test that doesn't call chrome.send('DialogClose').  And the hung renderer
dialog would interpret this as a wait timer restart.  So I avoid this by adding
a flag to limit the hung renderer dialog's ability to kill processes or restart
wait timers when instantiated from a test.  It's simpler than creating a full
controller & mock just to mute the one function.