Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2620963002:
MacViews: Correct combobox unittests so that they terminate correctly. (Closed)
|
Created:
3 years, 11 months ago by karandeepb Modified:
3 years, 11 months ago Reviewers:
tapted CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Correct combobox unittests so that they terminate correctly.
On MacViews, the tests ComboboxTest.NotifyOnClickWithSpaceKey and
ComboboxTest.NotifyOnClickWithReturnKey do not terminate currently when run
individually. This only happens on Mac, because on other platforms the combobox
dropdown menu is async (non-blocking). However on Mac, a native NSMenu is used
to show the combobox dropdown, which runs a nested message loop and hence is
blocking.
To solve, install a TestMenuRunner for all the tests using the ComboboxTest
fixture. This ensures that an actual menu is not created.
BUG=679980
TEST= Run out/Default/views_unittests
--gtest_filter="ComboboxTest.NotifyOnClickWithSpaceKey" on Mac. Ensure that the
test terminates normally.
Review-Url: https://codereview.chromium.org/2620963002
Cr-Commit-Position: refs/heads/master@{#443077}
Committed: https://chromium.googlesource.com/chromium/src/+/eff8bd896afe40d5d2646857939ed03b76ca3f12
Patch Set 1 : -- #
Total comments: 4
Patch Set 2 : Address review. #
Dependent Patchsets: Messages
Total messages: 30 (25 generated)
The CQ bit was checked by karandeepb@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.
The CQ bit was checked by karandeepb@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...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by karandeepb@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...
Description was changed from ========== Combobox correct tests. ========== to ========== MacViews: Correct combobox unittests so that they terminate correctly. On MacViews, the tests ComboboxTest.NotifyOnClickWithSpaceKey and ComboboxTest.NotifyOnClickWithReturnKey do not terminate currently when run individually. These pass when run in parallel with other tests, possibly because some other tests generate keystrokes which close the menu opened in these tests. This only happens on Mac, because on other platforms the combobox dropdown menu is async (non-blocking). However on Mac, a native NSMenu is used to show the combobox dropdown, which runs a nested message loop and hence is blocking. To solve, install a TestMenuRunner for all the tests using the ComboboxTest fixture. This ensures that an actual menu is not created. BUG=679980 ==========
Description was changed from ========== MacViews: Correct combobox unittests so that they terminate correctly. On MacViews, the tests ComboboxTest.NotifyOnClickWithSpaceKey and ComboboxTest.NotifyOnClickWithReturnKey do not terminate currently when run individually. These pass when run in parallel with other tests, possibly because some other tests generate keystrokes which close the menu opened in these tests. This only happens on Mac, because on other platforms the combobox dropdown menu is async (non-blocking). However on Mac, a native NSMenu is used to show the combobox dropdown, which runs a nested message loop and hence is blocking. To solve, install a TestMenuRunner for all the tests using the ComboboxTest fixture. This ensures that an actual menu is not created. BUG=679980 ========== to ========== MacViews: Correct combobox unittests so that they terminate correctly. On MacViews, the tests ComboboxTest.NotifyOnClickWithSpaceKey and ComboboxTest.NotifyOnClickWithReturnKey do not terminate currently when run individually. These pass when run in parallel with other tests, possibly because some other tests generate keystrokes which close the menu opened in these tests. This only happens on Mac, because on other platforms the combobox dropdown menu is async (non-blocking). However on Mac, a native NSMenu is used to show the combobox dropdown, which runs a nested message loop and hence is blocking. To solve, install a TestMenuRunner for all the tests using the ComboboxTest fixture. This ensures that an actual menu is not created. BUG=679980 TEST= Run out/Default/views_unittests --gtest_filter="ComboboxTest.NotifyOnClickWithSpaceKey" on Mac. Ensure that the test terminates normally. ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by karandeepb@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...
Description was changed from ========== MacViews: Correct combobox unittests so that they terminate correctly. On MacViews, the tests ComboboxTest.NotifyOnClickWithSpaceKey and ComboboxTest.NotifyOnClickWithReturnKey do not terminate currently when run individually. These pass when run in parallel with other tests, possibly because some other tests generate keystrokes which close the menu opened in these tests. This only happens on Mac, because on other platforms the combobox dropdown menu is async (non-blocking). However on Mac, a native NSMenu is used to show the combobox dropdown, which runs a nested message loop and hence is blocking. To solve, install a TestMenuRunner for all the tests using the ComboboxTest fixture. This ensures that an actual menu is not created. BUG=679980 TEST= Run out/Default/views_unittests --gtest_filter="ComboboxTest.NotifyOnClickWithSpaceKey" on Mac. Ensure that the test terminates normally. ========== to ========== MacViews: Correct combobox unittests so that they terminate correctly. On MacViews, the tests ComboboxTest.NotifyOnClickWithSpaceKey and ComboboxTest.NotifyOnClickWithReturnKey do not terminate currently when run individually. These pass when run in parallel with other tests, possibly because some other tests generate mouse or key events which close the menu opened in these tests. This only happens on Mac, because on other platforms the combobox dropdown menu is async (non-blocking). However on Mac, a native NSMenu is used to show the combobox dropdown, which runs a nested message loop and hence is blocking. To solve, install a TestMenuRunner for all the tests using the ComboboxTest fixture. This ensures that an actual menu is not created. BUG=679980 TEST= Run out/Default/views_unittests --gtest_filter="ComboboxTest.NotifyOnClickWithSpaceKey" on Mac. Ensure that the test terminates normally. ==========
Description was changed from ========== MacViews: Correct combobox unittests so that they terminate correctly. On MacViews, the tests ComboboxTest.NotifyOnClickWithSpaceKey and ComboboxTest.NotifyOnClickWithReturnKey do not terminate currently when run individually. These pass when run in parallel with other tests, possibly because some other tests generate mouse or key events which close the menu opened in these tests. This only happens on Mac, because on other platforms the combobox dropdown menu is async (non-blocking). However on Mac, a native NSMenu is used to show the combobox dropdown, which runs a nested message loop and hence is blocking. To solve, install a TestMenuRunner for all the tests using the ComboboxTest fixture. This ensures that an actual menu is not created. BUG=679980 TEST= Run out/Default/views_unittests --gtest_filter="ComboboxTest.NotifyOnClickWithSpaceKey" on Mac. Ensure that the test terminates normally. ========== to ========== MacViews: Correct combobox unittests so that they terminate correctly. On MacViews, the tests ComboboxTest.NotifyOnClickWithSpaceKey and ComboboxTest.NotifyOnClickWithReturnKey do not terminate currently when run individually. This only happens on Mac, because on other platforms the combobox dropdown menu is async (non-blocking). However on Mac, a native NSMenu is used to show the combobox dropdown, which runs a nested message loop and hence is blocking. To solve, install a TestMenuRunner for all the tests using the ComboboxTest fixture. This ensures that an actual menu is not created. BUG=679980 TEST= Run out/Default/views_unittests --gtest_filter="ComboboxTest.NotifyOnClickWithSpaceKey" on Mac. Ensure that the test terminates normally. ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Not quite sure what causes these tests to terminate when run in parallel with other tests, (don't think any views_unittest generates actual keystrokes/mouse events) since this means some other views_unittest is changing global state which is not correct.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nice cleanup! lgtm % nits But also is it weird that only running individually causes the tests to fail? Is it just that running stuff in parallel causes focus to be lost, which eventually dismisses the menu? https://codereview.chromium.org/2620963002/diff/60001/ui/views/controls/combo... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2620963002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:188: ComboboxTest() : widget_(NULL), combobox_(NULL), menu_show_count_(0) {} nit: move these to inline initializers, like widget_ = nullptr; etc. https://codereview.chromium.org/2620963002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:257: int menu_show_count() const { return menu_show_count_; } is this needed - it's OK for a test harness to have protected data members, and these are already that, so the test cases should just be able to read menu_show_count_ - I think that's nicer (e.g. they already access combobox_ so it's inconsistent otherwise)
Regarding >>But also is it weird that only running individually causes the tests to fail? Is >>it just that running stuff in parallel causes focus to be lost, which eventually >>dismisses the menu? Yeah I was also wondering the same in the comment with the earlier patch. As I had said, this implies some views_unittest is changing global state (via focus/key/mouse event etc) which is not correct. https://codereview.chromium.org/2620963002/diff/60001/ui/views/controls/combo... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2620963002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:188: ComboboxTest() : widget_(NULL), combobox_(NULL), menu_show_count_(0) {} On 2017/01/11 15:11:53, tapted wrote: > nit: move these to inline initializers, like > widget_ = nullptr; > > etc. Done. https://codereview.chromium.org/2620963002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:257: int menu_show_count() const { return menu_show_count_; } On 2017/01/11 15:11:53, tapted wrote: > is this needed - it's OK for a test harness to have protected data members, and > these are already that, so the test cases should just be able to read > menu_show_count_ - I think that's nicer (e.g. they already access combobox_ so > it's inconsistent otherwise) Oh yeah I should have declared menu_show_count_ private this way. Removed the getter.
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2620963002/#ps100001 (title: "Address review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484179287846880,
"parent_rev": "15b483f7dc564d1aaf60e8c400b03cf78ab2f7d5", "commit_rev":
"eff8bd896afe40d5d2646857939ed03b76ca3f12"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Correct combobox unittests so that they terminate correctly. On MacViews, the tests ComboboxTest.NotifyOnClickWithSpaceKey and ComboboxTest.NotifyOnClickWithReturnKey do not terminate currently when run individually. This only happens on Mac, because on other platforms the combobox dropdown menu is async (non-blocking). However on Mac, a native NSMenu is used to show the combobox dropdown, which runs a nested message loop and hence is blocking. To solve, install a TestMenuRunner for all the tests using the ComboboxTest fixture. This ensures that an actual menu is not created. BUG=679980 TEST= Run out/Default/views_unittests --gtest_filter="ComboboxTest.NotifyOnClickWithSpaceKey" on Mac. Ensure that the test terminates normally. ========== to ========== MacViews: Correct combobox unittests so that they terminate correctly. On MacViews, the tests ComboboxTest.NotifyOnClickWithSpaceKey and ComboboxTest.NotifyOnClickWithReturnKey do not terminate currently when run individually. This only happens on Mac, because on other platforms the combobox dropdown menu is async (non-blocking). However on Mac, a native NSMenu is used to show the combobox dropdown, which runs a nested message loop and hence is blocking. To solve, install a TestMenuRunner for all the tests using the ComboboxTest fixture. This ensures that an actual menu is not created. BUG=679980 TEST= Run out/Default/views_unittests --gtest_filter="ComboboxTest.NotifyOnClickWithSpaceKey" on Mac. Ensure that the test terminates normally. Review-Url: https://codereview.chromium.org/2620963002 Cr-Commit-Position: refs/heads/master@{#443077} Committed: https://chromium.googlesource.com/chromium/src/+/eff8bd896afe40d5d2646857939e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:100001) as https://chromium.googlesource.com/chromium/src/+/eff8bd896afe40d5d2646857939e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
