New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 755356 link

Starred by 1 user
Status: Archived
Owner:
xiy...@chromium.org
Closed: Aug 2017
Cc:
█ sonnysasaka@chromium.org
afakhry@chromium.org
sa...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Clean up simulated login code in ash_unittests

Project Member Reported by jamescook@chromium.org, Aug 14 2017 
Now that code in ash is using prefs, it seems like people are manually calling TestSessionControllerClient::SwitchActiveUser() to get OnActiveUserSessionChanged() to get prefs to show up.

https://cs.chromium.org/search/?q=f:ash.*test+SwitchActiveUser&sq=package:chromium&type=cs

It seems like this should all be handled via NoSessionAshTestBase and a single call to SetSessionStarted(true).

sammc has a CL that will make the prefs registration better (by providing a test PrefService) in ash unittests, see https://chromium-review.googlesource.com/c/605027 - but it would be best if there was only one way to simulate login.

Bonus points for:
* Add AshTestBase::SimulateLogin() and replace all the callers of SetSessionStarted(true) with it.

+cc a couple people who have CLs that use prefs in ash

Comment 1 by xiy...@chromium.org, Aug 15 2017 

AshTestBase::SetSessionStarted(true) calls TestSessionControllerClient::CreatePredefinedUserSessions. The problem of CreatePredefinedUserSessions, or say the difference between this method of production code, is that SetUserSessionOrder is not called. 

In production code, the SetUserSessionOrder for primary user (aka the first signed-in user) happens in SessionControllerClient::OnSessionStateChanged [1]. I put it there so that it executes when session state first becomes ACTIVE and user profile is fully loaded.

I have not tried it. A naive way would be add the missing SetUserSessionOrder call to CreatePredefinedUserSessions to eliminate the SwtichActiveUser work around.

[1]: https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/session_controller_client.cc?rcl=1343c373a69dc4acf7d1d0de37eaa9e0f2d316c4&l=438
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 21 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ffbc920300dd7ca747e2355adff726c955386e15

commit ffbc920300dd7ca747e2355adff726c955386e15
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Mon Aug 21 18:36:13 2017

ash: Clean up simulated login in ash_unittests

- Split AshTestBase::SetSessionStarted into SimulateLogin and
  ClearLogin;
- Remove deprecated AshTestBase::SetUserLoggedIna;
- Add AshTestBase::SimulateUserLogin to simulate a user login;
- Make TestSessionControllerClient::CreatePredefinedUserSessions
  to call SwitchActiveUser so that SessionController is updated
  with user session order to make its behavior closer to production
  code and notify observers about active user/pref change;
- Add SessionController::GetPrimaryUserSession and clean up
  AppListButton and its test;

BUG= 755356 
TEST=Existing ash_unittests

Change-Id: Ie3bba125f09622fb147a26fdca706aa72fec9a1d
Reviewed-on: https://chromium-review.googlesource.com/621404
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495998}
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/metrics/user_metrics_recorder_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/session/session_controller.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/session/session_controller.h
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/session/session_controller_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/session/test_session_controller_client.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/session/test_session_controller_client.h
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/shelf/app_list_button.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/shelf/app_list_button_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/shelf/shelf_controller_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/shelf/shelf_widget_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/shelf/shelf_window_watcher_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/system/lock_screen_action/lock_screen_action_tray_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/system/night_light/night_light_controller_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/system/night_light/tray_night_light_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/system/power/tablet_power_button_controller_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/system/session/logout_button_tray_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/system/session/logout_confirmation_controller_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/system/session/tray_session_length_limit_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/system/tiles/tray_tiles_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/system/user/tray_user_unittest.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/test/ash_test_base.cc
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/test/ash_test_base.h
[modify] https://crrev.com/ffbc920300dd7ca747e2355adff726c955386e15/ash/wm/lock_state_controller_unittest.cc

Comment 3 by xiy...@chromium.org, Aug 22 2017

Status: Fixed (was: Assigned)

Comment 4 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)
►

Sign in to add a comment