Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285023003:
Ensures the first browser in each interactive_ui_test is in the foreground (Closed)
DescriptionEnsures the first browser in each interactive_ui_test is in the foreground
It's entirely possible for an interactive_ui_test to do something that
triggers the suite to go to the background (such as clicking on a
desktop). By ensuring each browser is in the front at the start of the
test we avoid typical flake.
I should be able to remove some calls to BringBrowserWindowToFront
that are sprinkled throughout the code after this, but I'll do that
separately.
BUG=639350
TEST=test only fix
Committed: https://crrev.com/9a221a492c097da82c4f14019dad5dba14d9ac23
Cr-Commit-Position: refs/heads/master@{#415018}
Patch Set 1 #Patch Set 2 : do nothing on null browser #Patch Set 3 : not mac #
Total comments: 6
Patch Set 4 : feedback #
Messages
Total messages: 24 (18 generated)
The CQ bit was checked by sky@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sky@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Ensures the first browser in each interactive_ui_test is in the foreground It's entirely possible for an interactive_ui_test to do something that triggers the suite to go to the background (such as clicking on a desktop). By ensuring each browser is in the front at the start of the test we avoid typical flake. I should be able to remove some calls to BringBrowserWindowToFront that are sprinkled throughout the code after this, but I'll do that separately. BUG=639350 TEST=test only fix ========== to ========== Ensures the first browser in each interactive_ui_test is in the foreground It's entirely possible for an interactive_ui_test to do something that triggers the suite to go to the background (such as clicking on a desktop). By ensuring each browser is in the front at the start of the test we avoid typical flake. I should be able to remove some calls to BringBrowserWindowToFront that are sprinkled throughout the code after this, but I'll do that separately. BUG=639350 TEST=test only fix ==========
sky@chromium.org changed reviewers: + msw@chromium.org
I'm not sure what changed to make a handful of tests more likely to flake. It could be the number of interactive_ui_tests changed so that what tests run on which bot, or the ordering is different. Anyway, this seems to make the windows bots happy and is something we should be doing anyway.
The CQ bit was checked by sky@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.
LGTM, at least as a short-term or prospective fix. https://codereview.chromium.org/2285023003/diff/40001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/2285023003/diff/40001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:124: // first browser. This is intended to set up state applicable to all tests nit: s/first/supplied/ or given? https://codereview.chromium.org/2285023003/diff/40001/chrome/test/base/intera... File chrome/test/base/interactive_ui_tests_main.cc (right): https://codereview.chromium.org/2285023003/diff/40001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:85: // why. nit: file a bug? https://codereview.chromium.org/2285023003/diff/40001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:94: &ui_test_utils::BringBrowserWindowToFront); It seems odd that new browser windows would ever be created in the background or unactivated. Do we need a general fix for that? Otherwise, is it possible to call this somewhere from the InteractiveUITestSuiteRunner or similar, to avoid setting a function pointer?
https://codereview.chromium.org/2285023003/diff/40001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/2285023003/diff/40001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:124: // first browser. This is intended to set up state applicable to all tests On 2016/08/29 01:12:29, msw wrote: > nit: s/first/supplied/ or given? I changed 'on the' on the previous line to 'with the'. https://codereview.chromium.org/2285023003/diff/40001/chrome/test/base/intera... File chrome/test/base/interactive_ui_tests_main.cc (right): https://codereview.chromium.org/2285023003/diff/40001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:85: // why. On 2016/08/29 01:12:29, msw wrote: > nit: file a bug? Done. https://codereview.chromium.org/2285023003/diff/40001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:94: &ui_test_utils::BringBrowserWindowToFront); On 2016/08/29 01:12:29, msw wrote: > It seems odd that new browser windows would ever be created in the background or > unactivated. Do we need a general fix for that? Otherwise, is it possible to > call this somewhere from the InteractiveUITestSuiteRunner or similar, to avoid > setting a function pointer? Interactive tests some times test behavior when clicking on the desktop. When that happens the test launcher goes in the background. The next test that starts is then launched in the background and we get the flake we were getting. In other words I don't think it's unexpected that we need to do this, and you can see some tests have calls to BringBrowserWindowToFront already.
The CQ bit was checked by sky@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2285023003/#ps60001 (title: "feedback")
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 ========== Ensures the first browser in each interactive_ui_test is in the foreground It's entirely possible for an interactive_ui_test to do something that triggers the suite to go to the background (such as clicking on a desktop). By ensuring each browser is in the front at the start of the test we avoid typical flake. I should be able to remove some calls to BringBrowserWindowToFront that are sprinkled throughout the code after this, but I'll do that separately. BUG=639350 TEST=test only fix ========== to ========== Ensures the first browser in each interactive_ui_test is in the foreground It's entirely possible for an interactive_ui_test to do something that triggers the suite to go to the background (such as clicking on a desktop). By ensuring each browser is in the front at the start of the test we avoid typical flake. I should be able to remove some calls to BringBrowserWindowToFront that are sprinkled throughout the code after this, but I'll do that separately. BUG=639350 TEST=test only fix ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Ensures the first browser in each interactive_ui_test is in the foreground It's entirely possible for an interactive_ui_test to do something that triggers the suite to go to the background (such as clicking on a desktop). By ensuring each browser is in the front at the start of the test we avoid typical flake. I should be able to remove some calls to BringBrowserWindowToFront that are sprinkled throughout the code after this, but I'll do that separately. BUG=639350 TEST=test only fix ========== to ========== Ensures the first browser in each interactive_ui_test is in the foreground It's entirely possible for an interactive_ui_test to do something that triggers the suite to go to the background (such as clicking on a desktop). By ensuring each browser is in the front at the start of the test we avoid typical flake. I should be able to remove some calls to BringBrowserWindowToFront that are sprinkled throughout the code after this, but I'll do that separately. BUG=639350 TEST=test only fix Committed: https://crrev.com/9a221a492c097da82c4f14019dad5dba14d9ac23 Cr-Commit-Position: refs/heads/master@{#415018} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9a221a492c097da82c4f14019dad5dba14d9ac23 Cr-Commit-Position: refs/heads/master@{#415018} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
