Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1829603002:
MacViews: Fix flaky MenuRunnerCocoaTests. (Closed)
|
Created:
4 years, 9 months ago by karandeepb Modified:
4 years, 8 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Fix flaky MenuRunnerCocoaTests.
MenuRunnerCocoaTests are currently flaky. When run in parallel with other tests,
the blocks scheduled using CFRunLoopPerformBlock are sometimes not run, causing
the flakiness.
Ideally launching a menu should run the current thread's runloop in
NSEventTrackingRunLoopMode, causing the scheduled blocks to run. However, when
run in parallel with other tests involving UI operations, flakiness probably
occurs because the call to launch a menu may fail midway, causing the scheduled
blocks to not run. Instead of scheduling the block on a runloop, running them as
callbacks on SimpleMenuModel::Delegate::MenuWillShow fixes the flakiness.
BUG=521460
Committed: https://crrev.com/e8e08b359c588c9dd8ca4c5132c372ee6cbd2aaf
Cr-Commit-Position: refs/heads/master@{#385940}
Patch Set 1 #Patch Set 2 : Style improvements. #
Total comments: 6
Patch Set 3 : Using base::Callback. #
Total comments: 4
Patch Set 4 : Address review comments. #Messages
Total messages: 20 (9 generated)
Description was changed from ========== Format Initial commit BUG= ========== to ========== MacViews: Fix flaky MenuRunnerCocoaTests. MenuRunnerCocoaTests are currently flaky. When run in parallel with other tests, the blocks scheduled using CFRunLoopPerformBlock are sometimes not run, causing the flakiness. Ideally launching a menu should run the current thread's runloop in NSEventTrackingLoopMode, causing the scheduled blocks to run. However, when run in parallel with other tests involving UI operations, flakiness probably occurs because the call to launch a menu may fail midway, causing the scheduled blocks to not run. Instead of scheduling the block on a runloop, running them on a Menu open notification fixes the flakiness. BUG=521460 ==========
Description was changed from ========== MacViews: Fix flaky MenuRunnerCocoaTests. MenuRunnerCocoaTests are currently flaky. When run in parallel with other tests, the blocks scheduled using CFRunLoopPerformBlock are sometimes not run, causing the flakiness. Ideally launching a menu should run the current thread's runloop in NSEventTrackingLoopMode, causing the scheduled blocks to run. However, when run in parallel with other tests involving UI operations, flakiness probably occurs because the call to launch a menu may fail midway, causing the scheduled blocks to not run. Instead of scheduling the block on a runloop, running them on a Menu open notification fixes the flakiness. BUG=521460 ========== to ========== MacViews: Fix flaky MenuRunnerCocoaTests. MenuRunnerCocoaTests are currently flaky. When run in parallel with other tests, the blocks scheduled using CFRunLoopPerformBlock are sometimes not run, causing the flakiness. Ideally launching a menu should run the current thread's runloop in NSEventTrackingRunLoopMode, causing the scheduled blocks to run. However, when run in parallel with other tests involving UI operations, flakiness probably occurs because the call to launch a menu may fail midway, causing the scheduled blocks to not run. Instead of scheduling the block on a runloop, running them on a Menu open notification fixes the flakiness. BUG=521460 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Was there a specific reason why CFRunLoopPerformBlock was used earlier? Some other unit tests also use it, but the blocks they schedule don't involve any side effects. So it may happen that those blocks are not being run as well. Also, note that MenuRunnerCocoaTest.ComboboxAnchoring still flakes when multiple MenuRunnerCocoaTests are run in parallel(say in multiple shells). However, I can't get it to flake while running views_unittests. As i said in my CL, this is probably because opening a menu may fail midway when multiple parallel UI operations are occuring.
Yeah, I never really liked CFRunLoopPerformBlock -- if it's not necessary to test properly, then we should use idioms that more Chrome engineers will be familiar with. Sorry - it might be a bit more of an overhaul - see the wordy comment below. https://codereview.chromium.org/1829603002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_cocoa_unittest.mm (right): https://codereview.chromium.org/1829603002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:19: dispatch_block_t openCallback_; nit: @private https://codereview.chromium.org/1829603002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:36: if (self = [super init]) { nit: extra parens around assignment used as condition https://codereview.chromium.org/1829603002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:62: class TestModel : public ui::SimpleMenuModel { Can we override MenuModel::MenuWillShow? Here, or on the delegate (looks like either will work). I think that will make the code more readable for future spelunkers who might find ObjC block notation scary and intimidating. Also, Notifications are generally frowned upon as a logic control mechanism, since it makes the codepaths hard to follow. For example, it would be a good win to code health to remove kMenuControllerMenuWillOpenNotification. Looks like it's currently only used in browser_action_button_interactive_uitest.mm (so, additionally, it's also good to avoid code in production that would only be used for tests) And instead of dispatch_block_t, it would be nicer to use base::Callback (or base::Closure) with base::Bind, since more Chrome engineers will be familiar with that. https://codereview.chromium.org/1829603002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:142: runner_->RunMenuAt(parent_, NULL, gfx::Rect(), MENU_ANCHOR_TOPLEFT, nit (while you're here) NULL -> nullptr
Patchset #3 (id:40001) has been deleted
PTAL Trent. Thanks. https://codereview.chromium.org/1829603002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_cocoa_unittest.mm (right): https://codereview.chromium.org/1829603002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:62: class TestModel : public ui::SimpleMenuModel { On 2016/03/24 02:10:36, tapted wrote: > Can we override MenuModel::MenuWillShow? Here, or on the delegate (looks like > either will work). I think that will make the code more readable for future > spelunkers who might find ObjC block notation scary and intimidating. Also, > Notifications are generally frowned upon as a logic control mechanism, since it > makes the codepaths hard to follow. For example, it would be a good win to code > health to remove kMenuControllerMenuWillOpenNotification. Looks like it's > currently only used in browser_action_button_interactive_uitest.mm (so, > additionally, it's also good to avoid code in production that would only be used > for tests) > > And instead of dispatch_block_t, it would be nicer to use base::Callback (or > base::Closure) with base::Bind, since more Chrome engineers will be familiar > with that. Done. Though the code looks slightly uglier now since I can't use lambda functions. https://codereview.chromium.org/1829603002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:142: runner_->RunMenuAt(parent_, NULL, gfx::Rect(), MENU_ANCHOR_TOPLEFT, On 2016/03/24 02:10:36, tapted wrote: > nit (while you're here) NULL -> nullptr Done.
https://codereview.chromium.org/1829603002/diff/60001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_cocoa_unittest.mm (right): https://codereview.chromium.org/1829603002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:20: void MenuCancelCallback(internal::MenuRunnerImplCocoa* runner) { I think these all would be better as member functions on MenuRunnerCocoaTest. you just need to base base::Unretained(this) into base::Bind https://codereview.chromium.org/1829603002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:70: if (model_->menu_open_callback_.is_null()) nit: if (!model_->menu_open_callback_.is_null()) model_->menu_open_callback_.Run(); but also can we skip `if`? (is there a code path where we expect it to be null?)
PTAL Trent. https://codereview.chromium.org/1829603002/diff/60001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_cocoa_unittest.mm (right): https://codereview.chromium.org/1829603002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:20: void MenuCancelCallback(internal::MenuRunnerImplCocoa* runner) { On 2016/04/06 06:38:37, tapted wrote: > I think these all would be better as member functions on MenuRunnerCocoaTest. > you just need to base base::Unretained(this) into base::Bind Done. https://codereview.chromium.org/1829603002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_cocoa_unittest.mm:70: if (model_->menu_open_callback_.is_null()) On 2016/04/06 06:38:37, tapted wrote: > nit: > if (!model_->menu_open_callback_.is_null()) > model_->menu_open_callback_.Run(); > > but also can we skip `if`? (is there a code path where we expect it to be null?) Done.
lgtm
Description was changed from ========== MacViews: Fix flaky MenuRunnerCocoaTests. MenuRunnerCocoaTests are currently flaky. When run in parallel with other tests, the blocks scheduled using CFRunLoopPerformBlock are sometimes not run, causing the flakiness. Ideally launching a menu should run the current thread's runloop in NSEventTrackingRunLoopMode, causing the scheduled blocks to run. However, when run in parallel with other tests involving UI operations, flakiness probably occurs because the call to launch a menu may fail midway, causing the scheduled blocks to not run. Instead of scheduling the block on a runloop, running them on a Menu open notification fixes the flakiness. BUG=521460 ========== to ========== MacViews: Fix flaky MenuRunnerCocoaTests. MenuRunnerCocoaTests are currently flaky. When run in parallel with other tests, the blocks scheduled using CFRunLoopPerformBlock are sometimes not run, causing the flakiness. Ideally launching a menu should run the current thread's runloop in NSEventTrackingRunLoopMode, causing the scheduled blocks to run. However, when run in parallel with other tests involving UI operations, flakiness probably occurs because the call to launch a menu may fail midway, causing the scheduled blocks to not run. Instead of scheduling the block on a runloop, running them as callbacks on SimpleMenuModel::Delegate::MenuWillShow fixes the flakiness. BUG=521460 ==========
karandeepb@chromium.org changed reviewers: + sky@chromium.org
PTAL sky@. Thanks.
LGTM
The CQ bit was checked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829603002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829603002/80001
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix flaky MenuRunnerCocoaTests. MenuRunnerCocoaTests are currently flaky. When run in parallel with other tests, the blocks scheduled using CFRunLoopPerformBlock are sometimes not run, causing the flakiness. Ideally launching a menu should run the current thread's runloop in NSEventTrackingRunLoopMode, causing the scheduled blocks to run. However, when run in parallel with other tests involving UI operations, flakiness probably occurs because the call to launch a menu may fail midway, causing the scheduled blocks to not run. Instead of scheduling the block on a runloop, running them as callbacks on SimpleMenuModel::Delegate::MenuWillShow fixes the flakiness. BUG=521460 ========== to ========== MacViews: Fix flaky MenuRunnerCocoaTests. MenuRunnerCocoaTests are currently flaky. When run in parallel with other tests, the blocks scheduled using CFRunLoopPerformBlock are sometimes not run, causing the flakiness. Ideally launching a menu should run the current thread's runloop in NSEventTrackingRunLoopMode, causing the scheduled blocks to run. However, when run in parallel with other tests involving UI operations, flakiness probably occurs because the call to launch a menu may fail midway, causing the scheduled blocks to not run. Instead of scheduling the block on a runloop, running them as callbacks on SimpleMenuModel::Delegate::MenuWillShow fixes the flakiness. BUG=521460 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix flaky MenuRunnerCocoaTests. MenuRunnerCocoaTests are currently flaky. When run in parallel with other tests, the blocks scheduled using CFRunLoopPerformBlock are sometimes not run, causing the flakiness. Ideally launching a menu should run the current thread's runloop in NSEventTrackingRunLoopMode, causing the scheduled blocks to run. However, when run in parallel with other tests involving UI operations, flakiness probably occurs because the call to launch a menu may fail midway, causing the scheduled blocks to not run. Instead of scheduling the block on a runloop, running them as callbacks on SimpleMenuModel::Delegate::MenuWillShow fixes the flakiness. BUG=521460 ========== to ========== MacViews: Fix flaky MenuRunnerCocoaTests. MenuRunnerCocoaTests are currently flaky. When run in parallel with other tests, the blocks scheduled using CFRunLoopPerformBlock are sometimes not run, causing the flakiness. Ideally launching a menu should run the current thread's runloop in NSEventTrackingRunLoopMode, causing the scheduled blocks to run. However, when run in parallel with other tests involving UI operations, flakiness probably occurs because the call to launch a menu may fail midway, causing the scheduled blocks to not run. Instead of scheduling the block on a runloop, running them as callbacks on SimpleMenuModel::Delegate::MenuWillShow fixes the flakiness. BUG=521460 Committed: https://crrev.com/e8e08b359c588c9dd8ca4c5132c372ee6cbd2aaf Cr-Commit-Position: refs/heads/master@{#385940} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e8e08b359c588c9dd8ca4c5132c372ee6cbd2aaf Cr-Commit-Position: refs/heads/master@{#385940} |
