|
|
Created:
5 years, 10 months ago by afakhry Modified:
5 years, 10 months ago Reviewers:
Nikita (slow) CC:
chromium-reviews, dzhioev+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAsh Tray Browser Test (Retry)
This to make sure that after login, the system tray is always visible
and within the bounds of the screen.
R=nkostylev@chromium.org
BUG=372838
TEST=interactive_ui_tests --gtest_filter=Login*Test.*
Committed: https://crrev.com/b90079d8f837477dfd1ea8468a5823ee56125cb8
Cr-Commit-Position: refs/heads/master@{#318029}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #Messages
Total messages: 14 (1 generated)
https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/login_browsertest.cc (right): https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_browsertest.cc:148: class LoginManagerTest : public InProcessBrowserTest { Please rename this class to something else since LoginManagerTest is also a name of a base class that is used in multiple tests. https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_browsertest.cc:151: command_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests); As discussed over IM you need both kForceLoginManagerInTests and kLoginManager switches in tests. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/login_browsertest.cc (right): https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_browsertest.cc:151: command_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests); On 2015/02/13 12:16:17, Nikita wrote: > As discussed over IM you need both kForceLoginManagerInTests and kLoginManager > switches in tests. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... I thought you said having kForceLoginManagerInTests is enough. In this case having both switches will cause the system tray test to always fail! So what do you think we should do?
On 2015/02/13 16:59:49, afakhry wrote: > https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... > File chrome/browser/chromeos/login/login_browsertest.cc (right): > > https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/login_browsertest.cc:151: > command_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests); > On 2015/02/13 12:16:17, Nikita wrote: > > As discussed over IM you need both kForceLoginManagerInTests and kLoginManager > > switches in tests. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > I thought you said having kForceLoginManagerInTests is enough. In this case > having both switches will cause the system tray test to always fail! So what do > you think we should do? I still don't understand why tray is not visible in this configuration. One way to figure out that would be to run test locally with --enable-pixel-output-in-tests and let it run in the end so that you can check visual output.
On 2015/02/13 17:55:54, Nikita wrote: > On 2015/02/13 16:59:49, afakhry wrote: > > > https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... > > File chrome/browser/chromeos/login/login_browsertest.cc (right): > > > > > https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... > > chrome/browser/chromeos/login/login_browsertest.cc:151: > > command_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests); > > On 2015/02/13 12:16:17, Nikita wrote: > > > As discussed over IM you need both kForceLoginManagerInTests and > kLoginManager > > > switches in tests. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > I thought you said having kForceLoginManagerInTests is enough. In this case > > having both switches will cause the system tray test to always fail! So what > do > > you think we should do? > > I still don't understand why tray is not visible in this configuration. > One way to figure out that would be to run test locally with > --enable-pixel-output-in-tests and let it run in the end so that you can check > visual output. --enable-pixel-output-in-tests doesn't help me to see anything unfortunately ... it's just a black screen.
On 2015/02/14 00:19:20, afakhry wrote: > On 2015/02/13 17:55:54, Nikita wrote: > > On 2015/02/13 16:59:49, afakhry wrote: > > > > > > https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... > > > File chrome/browser/chromeos/login/login_browsertest.cc (right): > > > > > > > > > https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... > > > chrome/browser/chromeos/login/login_browsertest.cc:151: > > > command_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests); > > > On 2015/02/13 12:16:17, Nikita wrote: > > > > As discussed over IM you need both kForceLoginManagerInTests and > > kLoginManager > > > > switches in tests. > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > > > I thought you said having kForceLoginManagerInTests is enough. In this case > > > having both switches will cause the system tray test to always fail! So what > > do > > > you think we should do? > > > > I still don't understand why tray is not visible in this configuration. > > One way to figure out that would be to run test locally with > > --enable-pixel-output-in-tests and let it run in the end so that you can check > > visual output. > > --enable-pixel-output-in-tests doesn't help me to see anything unfortunately ... > it's just a black screen. That's strange, since it is currently the only switch as far as I know required for such visual debugging. What was the cmd line you're were running?
On 2015/02/16 08:12:03, Nikita wrote: > On 2015/02/14 00:19:20, afakhry wrote: > > On 2015/02/13 17:55:54, Nikita wrote: > > > On 2015/02/13 16:59:49, afakhry wrote: > > > > > > > > > > https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... > > > > File chrome/browser/chromeos/login/login_browsertest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/922463005/diff/1/chrome/browser/chromeos/logi... > > > > chrome/browser/chromeos/login/login_browsertest.cc:151: > > > > command_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests); > > > > On 2015/02/13 12:16:17, Nikita wrote: > > > > > As discussed over IM you need both kForceLoginManagerInTests and > > > kLoginManager > > > > > switches in tests. > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > > > > > I thought you said having kForceLoginManagerInTests is enough. In this > case > > > > having both switches will cause the system tray test to always fail! So > what > > > do > > > > you think we should do? > > > > > > I still don't understand why tray is not visible in this configuration. > > > One way to figure out that would be to run test locally with > > > --enable-pixel-output-in-tests and let it run in the end so that you can > check > > > visual output. > > > > --enable-pixel-output-in-tests doesn't help me to see anything unfortunately > ... > > it's just a black screen. > > That's strange, since it is currently the only switch as far as I know required > for such visual debugging. > What was the cmd line you're were running? Just "interactive_ui_tests --gtest_filter=LoginManagerTest.SysTrayIsVisible"
I think these tests are pretty much adequate to make sure that the regression mentioned in the bug doesn't happen again.
lgtm
The CQ bit was checked by nkostylev@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/922463005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b90079d8f837477dfd1ea8468a5823ee56125cb8 Cr-Commit-Position: refs/heads/master@{#318029} |