Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8418031:
Regression tests for two recently-fixed audio-related crashers. (Closed)
|
Created:
9 years, 1 month ago by Ami GONE FROM CHROMIUM Modified:
9 years, 1 month ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., acolwell+watch_chromium.org, annacc+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org, enal Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRegression tests for two recently-fixed audio-related crashers.
BUG=101228, 101375
TEST=new tests pass locally, trybots
Patch Set 1 : . #
Total comments: 17
Patch Set 2 : . #
Total comments: 13
Messages
Total messages: 8 (0 generated)
Eugene: purely FYI.
nits but LGTM http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... File chrome/browser/media/media_browsertest.cc (right): http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:17: FilePath("media"), FilePath("seek-jumper.html"))) {} indent +2 pedantic nit 'o mine: if entire ctor/dtor/fn + {} line doesn't fit on one line, drop the } http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:17: FilePath("media"), FilePath("seek-jumper.html"))) {} fix file path literals http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:20: EnableDOMAutomation(); sanity check: can we not move this to ctor then avoid calling IPBTest::SetUp() ? http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:61: IN_PROC_BROWSER_TEST_F(MediaBrowserTest, SeekJumperAlone) { nit: for prefixed test names we've typically added a _ to separate the cases SeekJumper_Alone SeekJumper_SharedRenderer http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:71: remove blank line http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:76: remove blank line http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:91: FROM_HERE, new MessageLoop::QuitTask(), 2000); test timeouts? http://codereview.chromium.org/8418031/diff/2001/chrome/test/data/media/seek-... File chrome/test/data/media/seek-jumper.html (right): http://codereview.chromium.org/8418031/diff/2001/chrome/test/data/media/seek-... chrome/test/data/media/seek-jumper.html:4: blank line http://codereview.chromium.org/8418031/diff/2001/chrome/test/data/media/seek-... chrome/test/data/media/seek-jumper.html:9: a.currentTime = Math.random() * a.duration; is this unseeded?
http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... File chrome/browser/media/media_browsertest.cc (right): http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:17: FilePath("media"), FilePath("seek-jumper.html"))) {} On 2011/10/28 21:03:13, scherkus wrote: > indent +2 I don't think so - as a continuation line it indents +4 from the previous line's first char, which in this case is a colon. > > pedantic nit 'o mine: if entire ctor/dtor/fn + {} line doesn't fit on one line, > drop the } http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:20: EnableDOMAutomation(); On 2011/10/28 21:03:13, scherkus wrote: > sanity check: can we not move this to ctor then avoid calling IPBTest::SetUp() ? Done. http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:61: IN_PROC_BROWSER_TEST_F(MediaBrowserTest, SeekJumperAlone) { On 2011/10/28 21:03:13, scherkus wrote: > nit: for prefixed test names we've typically added a _ to separate the cases > > SeekJumper_Alone > SeekJumper_SharedRenderer Done. http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:71: On 2011/10/28 21:03:13, scherkus wrote: > remove blank line Done. http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:76: On 2011/10/28 21:03:13, scherkus wrote: > remove blank line Done. http://codereview.chromium.org/8418031/diff/2001/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:91: FROM_HERE, new MessageLoop::QuitTask(), 2000); On 2011/10/28 21:03:13, scherkus wrote: > test timeouts? I don't think so; none of them are a good fit. Note that this *always* waits this long; it's not a timeout value. Hence the "Sad" in the preceding comment. http://codereview.chromium.org/8418031/diff/2001/chrome/test/data/media/seek-... File chrome/test/data/media/seek-jumper.html (right): http://codereview.chromium.org/8418031/diff/2001/chrome/test/data/media/seek-... chrome/test/data/media/seek-jumper.html:4: On 2011/10/28 21:03:13, scherkus wrote: > blank line Done. http://codereview.chromium.org/8418031/diff/2001/chrome/test/data/media/seek-... chrome/test/data/media/seek-jumper.html:9: a.currentTime = Math.random() * a.duration; On 2011/10/28 21:03:13, scherkus wrote: > is this unseeded? It is seeded randomly by current time. I don't think JS has a seedable RNG.
My inability to get the bots to reliably run these tests made me give up. Leaving issue in rietveld for posterity (and as a reference for http://crbug.com/102395 which this seems to have found).
Drive-by, in which I show you likely reasons why the test is not reliable. Please let me know if I can help you with solving those problems. They should be solvable, all of this is generally known and "common". http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... File chrome/browser/media/media_browsertest.cc (right): http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:25: void WaitForSuccess(const base::Callback<bool(void)>& closure) { Sorry, no way. Please wait for an event, this kind of code is a known anti-pattern. http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:47: CHECK(ui_test_utils::ExecuteJavaScriptAndExtractBool( Please avoid CHECK, on any failures the test would have to be DISABLED. I can help you come up with a better design. http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:65: CHECK(ui_test_utils::SendKeyPressSync( Same here. http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:85: // Give the renderer a bit of time to crash. Sad but necessary. Not true, at least on POSIX. On Windows it's a known problem that should be fixed. No workarounds please. http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:87: FROM_HERE, new MessageLoop::QuitTask(), 2000); No hardcoded timeouts please.
Thanks for looking, but I think you think the tests are unreliable because I'm being insufficiently patient with my timeouts, or something. Replacing TestTimeouts::action_timeout_ms() (10s) with TestTimeouts::action_max_timeout_ms() (45s) didn't help reliability on the trybots, and anyway the things being waited for should trigger in well under a second. Please look at the trybot results for the different patches as they show multiple problems: the mac bots revealed http://crbug.com/102395, the linux bots seem to have problems opening sound cards (do they have sound cards?) and the windows bots time out... http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... File chrome/browser/media/media_browsertest.cc (right): http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:25: void WaitForSuccess(const base::Callback<bool(void)>& closure) { On 2011/11/02 08:43:27, Paweł Hajdan Jr. wrote: > Sorry, no way. Please wait for an event, this kind of code is a known > anti-pattern. The events I need to await for are "JS expression becomes true in the page" (or, equivalently, JS method is called in the page) and "browser tab count becomes N". Can you tell me how to listen for them without these effective sleeps? http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:47: CHECK(ui_test_utils::ExecuteJavaScriptAndExtractBool( On 2011/11/02 08:43:27, Paweł Hajdan Jr. wrote: > Please avoid CHECK, on any failures the test would have to be DISABLED. I can > help you come up with a better design. 1) Why does a CHECK (as opposed to propagating a gtest failure and ASSERTing it's absence in the TEST method) subject the test to a higher risk of DISABLEment? I thought InProcessBrowserTest's were launched in individual processes anyway, so a CHECK failure here results in just the calling test crashing, and not affecting other tests. 2) The condition being CHECKd for indicates a programming error, so it seems reasonable to me to use CHECK (as opposed to missing an expectation of the test, for which I use ASSERT/EXPECT). http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:85: // Give the renderer a bit of time to crash. Sad but necessary. On 2011/11/02 08:43:27, Paweł Hajdan Jr. wrote: > Not true, at least on POSIX. On Windows it's a known problem that should be > fixed. No workarounds please. I feel like you have a magic cure but you're holding it behind your back, waiting for me to guess :) How would you implement this on POSIX? How would you fix this on Windows? http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:87: FROM_HERE, new MessageLoop::QuitTask(), 2000); On 2011/11/02 08:43:27, Paweł Hajdan Jr. wrote: > No hardcoded timeouts please. Have you looked at the bugs that these tests are regression-testing? I'm happy to hear concrete suggestions you have for improvements.
One note: I _don't_ think it's because timeouts you're using are too short. One of the reasons TestTimeouts were created was to avoid people trying to increase custom timeouts slightly to try to fix flakiness. Now we have large enough timeouts that should work for everyone. It seems to me the primary reason of problems here is the C++ <-> JS interaction, and the WaitForSuccess loop. This is non-trivial of couse, but I thought I'd share my thoughts anyway (in my initial review comments I didn't really see your note that this change is not being committed for now, so I've done everything similarly to any other drive-by review). http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... File chrome/browser/media/media_browsertest.cc (right): http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:25: void WaitForSuccess(const base::Callback<bool(void)>& closure) { On 2011/11/02 16:32:48, Ami Fischman wrote: > The events I need to await for are "JS expression becomes true in the page" (or, > equivalently, JS method is called in the page) This is non-trivial, but I think extensions and WebUI teams do something very similar. Unfortunately I don't know much more. However, this C++ <-> JS interaction is a very common cause of flakiness if not done with extreme care. > "browser tab count becomes N". Can you tell me how to listen > for them without these effective sleeps? That's easy! See TabCountChangeObserver and http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome... It may even be useful for you in the future, so I'm glad you asked. http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:47: CHECK(ui_test_utils::ExecuteJavaScriptAndExtractBool( On 2011/11/02 16:32:48, Ami Fischman wrote: > 1) Why does a CHECK (as opposed to propagating a gtest failure and ASSERTing > it's absence in the TEST method) subject the test to a higher risk of > DISABLEment? I thought InProcessBrowserTest's were launched in individual > processes anyway, so a CHECK failure here results in just the calling test > crashing, and not affecting other tests. That's true. Now here's the catch: we still want to distinguish between flaky failures and crashes, so FLAKY and FAILS do not suppress those crashes. Other tests are indeed not affected, but the only way to green the tree is to DISABLE the test. Please see also http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/han... > 2) The condition being CHECKd for indicates a programming error, so it seems > reasonable to me to use CHECK (as opposed to missing an expectation of the test, > for which I use ASSERT/EXPECT). For anything in ui_test_utils failing, you can't say for sure it's a programming error. There are many different reasons those calls can fail, especially those related to JS and the renderers. http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:85: // Give the renderer a bit of time to crash. Sad but necessary. On 2011/11/02 16:32:48, Ami Fischman wrote: > I feel like you have a magic cure but you're holding it behind your back, > waiting for me to guess :) Not exactly, but yeah, I was working on this issue and just wanted to keep my initial comment short. I'm glad you're interested in the real answer. > How would you implement this on POSIX? Take a look at InitialLoadObserver and content::NOTIFICATION_RENDERER_PROCESS_CLOSED. One note: you should then probably check the details to see if it was a crash. You can then use WindowedNotificationObserver to wait for the notification. > How would you fix this on Windows? Same way, but I think there is a bug that results in NOTIFICATION_RENDERER_PROCESS_CLOSED not being called in some circumstances (when the renderer crashes before connecting to the IPC channel). Actually this test doesn't seem to be affected by that bug. http://codereview.chromium.org/8418031/diff/1005/chrome/browser/media/media_b... chrome/browser/media/media_browsertest.cc:87: FROM_HERE, new MessageLoop::QuitTask(), 2000); On 2011/11/02 16:32:48, Ami Fischman wrote: > On 2011/11/02 08:43:27, Paweł Hajdan Jr. wrote: > > No hardcoded timeouts please. > > Have you looked at the bugs that these tests are regression-testing? I'm happy > to hear concrete suggestions you have for improvements. I've read the bugs now. I think my comment about this still makes sense. On Valgrind you'll execute much less iterations in 2 s than without it. With TestTimeouts this time would be automatically adjusted for you when executing in a slower environment. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
