Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259353002:
Update Flaky ToolbarActionViewInteractiveUITests (Closed)
DescriptionUpdate Flaky ToolbarActionViewInteractiveUITests
Update ActivateOverflowedToolbarActionWithKeyboard to block on final keyboard
event, then clear the pending messages, before confirming the final state.
Update TestContextMenuOnOverflowedAction. Remove the concept of the context
menu callback, this is no longer needed with the menus not nesting message
loops. Instead of posting tasks to the message loop in order to continue the
test, they will be called directly. The message loop will still be ran until
idle in order to allow for the processing of UI events.
TEST=
ToolbarActionViewInteractiveUITests.ActivateOverflowedToolbarActionWithKeyboard,
ToolbarActionViewInteractiveUITests.TestContextMenuOnOverflowedAction
BUG=638692, 639010
Committed: https://crrev.com/cd6568bdc044afe156d1bf4bb29cb3be213d615a
Cr-Commit-Position: refs/heads/master@{#414549}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase #
Total comments: 8
Patch Set 4 : Updates #
Total comments: 8
Patch Set 5 : Nits #
Messages
Total messages: 27 (16 generated)
Patchset #1 (id:1) has been deleted
jonross@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hey Devlin, Could you take a look at this change? I've updated the flaking tests. TestContextMenuOnOverflowedAction was flaking as the tasks posted by the context menu callback could end up triggering additional work after the main test had finished working. I've removed the callback as we can operate on the ToolbarActionView directly from the test since message loops aren't nested. I've also updated the blocking in ActivateOverflowedToolbarActionWithKeyboard. This should clear the message loop after receiving the event back from the platform.
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Looks like these might still be failing on the bots? Also, please wrap your CL description to be a git-logging-friendly 72 chars :) (or at most 80)
On 2016/08/20 01:17:00, Devlin wrote: > Looks like these might still be failing on the bots? > > Also, please wrap your CL description to be a git-logging-friendly 72 chars :) > (or at most 80) Interesting, the updated tests are now failing with timeouts. Similar to the rest of the block found in crbug.com/639350
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Update Flaky ToolbarActionViewInteractiveUITests
Update ActivateOverflowedToolbarActionWithKeyboard to block on final keyboard
event, then clear the pending messages, before confirming the final state.
Update TestContextMenuOnOverflowedAction. Remove the concept of the context menu
callback, this is no longer needed with the menus not nesting message loops.
Instead of posting tasks to the message loop in order to continue the test, they
will be called directly. The message loop will still be ran until idle in order
to allow for the processing of UI events.
TEST=ToolbarActionViewInteractiveUITests.ActivateOverflowedToolbarActionWithKeyboard,
ToolbarActionViewInteractiveUITests.TestContextMenuOnOverflowedAction
BUG=638692, 639010
==========
to
==========
Update ActivateOverflowedToolbarActionWithKeyboard to block on final keyboard
event, then clear the pending messages, before confirming the final state.
Update TestContextMenuOnOverflowedAction. Remove the concept of the context
menu callback, this is no longer needed with the menus not nesting message
loops. Instead of posting tasks to the message loop in order to continue the
test, they will be called directly. The message loop will still be ran until
idle in order to allow for the processing of UI events.
TEST=
ToolbarActionViewInteractiveUITests.ActivateOverflowedToolbarActionWithKeyboard,
ToolbarActionViewInteractiveUITests.TestContextMenuOnOverflowedAction
BUG=638692, 639010
==========
I've re-wrapped the description. What are your thoughts on the updates to these two tests. They do not address the extension loading error, nor the timeouts seen. However they do address race conditions between event posting, and subsequent testing.
lgtm. I'm investigating the extension loading error separately, but it shouldn't block this. Note also that the first line of the "description" in rietveld becomes the commit message, so you should duplicate the "Update Flaky ToolbarActionViewInteractiveUITests" line there. i.e. Title: Update Flaky ToolbarActionViewInteractiveUITests Description: Update Flaky ToolbarActionViewInteractiveUITests Update ActivateOverflowedToolbarActionWithKeyboard to block... (this happens automatically when you upload the cl, but is easy to miss when you're updating the description) https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.h (right): https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.h:88: bool IsMenuRunning() const override; I'd slightly prefer this wrapped in a ForTesting(). https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc (right): https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:42: // |button| specifies the mouse button to click with. Returning the targetted s/Returning/Returns https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:42: // |button| specifies the mouse button to click with. Returning the targetted typo: targetted https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:51: EXPECT_TRUE(app_menu); The reason these were asserts is because if they're false, we segfault. I think they should remain asserts. :)
Description was changed from
==========
Update ActivateOverflowedToolbarActionWithKeyboard to block on final keyboard
event, then clear the pending messages, before confirming the final state.
Update TestContextMenuOnOverflowedAction. Remove the concept of the context
menu callback, this is no longer needed with the menus not nesting message
loops. Instead of posting tasks to the message loop in order to continue the
test, they will be called directly. The message loop will still be ran until
idle in order to allow for the processing of UI events.
TEST=
ToolbarActionViewInteractiveUITests.ActivateOverflowedToolbarActionWithKeyboard,
ToolbarActionViewInteractiveUITests.TestContextMenuOnOverflowedAction
BUG=638692, 639010
==========
to
==========
Update Flaky ToolbarActionViewInteractiveUITests
Update ActivateOverflowedToolbarActionWithKeyboard to block on final keyboard
event, then clear the pending messages, before confirming the final state.
Update TestContextMenuOnOverflowedAction. Remove the concept of the context
menu callback, this is no longer needed with the menus not nesting message
loops. Instead of posting tasks to the message loop in order to continue the
test, they will be called directly. The message loop will still be ran until
idle in order to allow for the processing of UI events.
TEST=
ToolbarActionViewInteractiveUITests.ActivateOverflowedToolbarActionWithKeyboard,
ToolbarActionViewInteractiveUITests.TestContextMenuOnOverflowedAction
BUG=638692, 639010
==========
https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.h (right): https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.h:88: bool IsMenuRunning() const override; On 2016/08/25 16:42:12, Devlin wrote: > I'd slightly prefer this wrapped in a ForTesting(). Done. I've placed the actual override back in private. https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc (right): https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:42: // |button| specifies the mouse button to click with. Returning the targetted On 2016/08/25 16:42:12, Devlin wrote: > typo: targetted Done. https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:42: // |button| specifies the mouse button to click with. Returning the targetted On 2016/08/25 16:42:12, Devlin wrote: > s/Returning/Returns Done. https://codereview.chromium.org/2259353002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:51: EXPECT_TRUE(app_menu); On 2016/08/25 16:42:12, Devlin wrote: > The reason these were asserts is because if they're false, we segfault. I think > they should remain asserts. :) I removed these as they interfere with return values. I've restored them for the early segfaults, and switched to an optional out param
a few last nits, otherwise slgtm https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.h (right): https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.h:88: bool IsMenuRunningForTesting() const; nit: put this near the other testing methods. https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc (right): https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:43: // ToolbarActionView. Optionally |toolbar_action_view| can be provided to This no longer returns the targeted view, right? https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:60: ToolbarActionView *action_view = this was right before https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:259: ToolbarActionView* action_view; initialize to nullptr
https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.h (right): https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.h:88: bool IsMenuRunningForTesting() const; On 2016/08/25 18:41:44, Devlin wrote: > nit: put this near the other testing methods. Done. https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc (right): https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:43: // ToolbarActionView. Optionally |toolbar_action_view| can be provided to On 2016/08/25 18:41:44, Devlin wrote: > This no longer returns the targeted view, right? oooops https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:60: ToolbarActionView *action_view = On 2016/08/25 18:41:44, Devlin wrote: > this was right before Done. https://codereview.chromium.org/2259353002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:259: ToolbarActionView* action_view; On 2016/08/25 18:41:44, Devlin wrote: > initialize to nullptr Done.
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2259353002/#ps100001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Update Flaky ToolbarActionViewInteractiveUITests
Update ActivateOverflowedToolbarActionWithKeyboard to block on final keyboard
event, then clear the pending messages, before confirming the final state.
Update TestContextMenuOnOverflowedAction. Remove the concept of the context
menu callback, this is no longer needed with the menus not nesting message
loops. Instead of posting tasks to the message loop in order to continue the
test, they will be called directly. The message loop will still be ran until
idle in order to allow for the processing of UI events.
TEST=
ToolbarActionViewInteractiveUITests.ActivateOverflowedToolbarActionWithKeyboard,
ToolbarActionViewInteractiveUITests.TestContextMenuOnOverflowedAction
BUG=638692, 639010
==========
to
==========
Update Flaky ToolbarActionViewInteractiveUITests
Update ActivateOverflowedToolbarActionWithKeyboard to block on final keyboard
event, then clear the pending messages, before confirming the final state.
Update TestContextMenuOnOverflowedAction. Remove the concept of the context
menu callback, this is no longer needed with the menus not nesting message
loops. Instead of posting tasks to the message loop in order to continue the
test, they will be called directly. The message loop will still be ran until
idle in order to allow for the processing of UI events.
TEST=
ToolbarActionViewInteractiveUITests.ActivateOverflowedToolbarActionWithKeyboard,
ToolbarActionViewInteractiveUITests.TestContextMenuOnOverflowedAction
BUG=638692, 639010
==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
Update Flaky ToolbarActionViewInteractiveUITests
Update ActivateOverflowedToolbarActionWithKeyboard to block on final keyboard
event, then clear the pending messages, before confirming the final state.
Update TestContextMenuOnOverflowedAction. Remove the concept of the context
menu callback, this is no longer needed with the menus not nesting message
loops. Instead of posting tasks to the message loop in order to continue the
test, they will be called directly. The message loop will still be ran until
idle in order to allow for the processing of UI events.
TEST=
ToolbarActionViewInteractiveUITests.ActivateOverflowedToolbarActionWithKeyboard,
ToolbarActionViewInteractiveUITests.TestContextMenuOnOverflowedAction
BUG=638692, 639010
==========
to
==========
Update Flaky ToolbarActionViewInteractiveUITests
Update ActivateOverflowedToolbarActionWithKeyboard to block on final keyboard
event, then clear the pending messages, before confirming the final state.
Update TestContextMenuOnOverflowedAction. Remove the concept of the context
menu callback, this is no longer needed with the menus not nesting message
loops. Instead of posting tasks to the message loop in order to continue the
test, they will be called directly. The message loop will still be ran until
idle in order to allow for the processing of UI events.
TEST=
ToolbarActionViewInteractiveUITests.ActivateOverflowedToolbarActionWithKeyboard,
ToolbarActionViewInteractiveUITests.TestContextMenuOnOverflowedAction
BUG=638692, 639010
Committed: https://crrev.com/cd6568bdc044afe156d1bf4bb29cb3be213d615a
Cr-Commit-Position: refs/heads/master@{#414549}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cd6568bdc044afe156d1bf4bb29cb3be213d615a Cr-Commit-Position: refs/heads/master@{#414549} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
