[Night Light] CL2: Ash and system tray work

ash/system/night_light/night_light_unittest.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.

[James Cook, 2017/05/05 17:12:32]

I would put this file in night_light_controller_unittest.cc, even though it's
testing the whole feature.

[afakhry, 2017/05/05 20:04:11]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/public/cpp/config.h"
+#include "ash/public/cpp/session_types.h"
+#include "ash/shell.h"
+#include "ash/system/night_light/night_light_controller.h"
+#include "ash/test/ash_test_base.h"
+#include "ash/test/ash_test_helper.h"
+#include "ash/test/test_session_controller_client.h"
+#include "ash/test/test_shell_delegate.h"
+#include "components/prefs/testing_pref_service.h"
+#include "ui/compositor/layer.h"
+
+namespace ash {
+
+namespace {
+
+constexpr char user_1_email[] = "user1@nightlight";
+constexpr char user_2_email[] = "user2@nightlight";
+
+NightLightController* GetController() {
+  return Shell::Get()->night_light_controller();
+}
+
+// Tests that the color temperatures of all root layers are equal to the given
+// |expected_temperature| and returns true if so, or false otherwise.
+bool TestLayersTemperature(float expected_temperature) {
+  for (aura::Window* root_window : ash::Shell::GetAllRootWindows()) {

[James Cook, 2017/05/05 17:12:32]

By default AshTestBase only creates a single display and root window. Either use
GetPrimaryRootWindow() and don't iterate, or use
UpdateDisplays("800x600,800x600") and test the multi-display case.

[afakhry, 2017/05/05 20:04:11]

Oh, I like UpdateDisplay(). That makes the tests more rigorous since the
expectation is that all displays should be updated.

Done.

+    ui::Layer* layer = root_window->layer();
+    if (expected_temperature != layer->layer_temperature())
+      return false;
+  }
+
+  return true;
+}
+
+class TestObserver : public NightLightController::Observer {
+ public:
+  TestObserver() { GetController()->AddObserver(this); }
+  ~TestObserver() override { GetController()->RemoveObserver(this); }
+
+  // ash::NightLightController::Observer:
+  void OnStatusChanged(bool new_status) override { status_ = new_status; }
+
+  bool status() const { return status_; }
+
+ private:
+  bool status_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(TestObserver);
+};
+
+class NightLightTest : public test::AshTestBase {
+ public:
+  NightLightTest() {}

[James Cook, 2017/05/05 17:12:32]

nit: = default, and below

[afakhry, 2017/05/05 20:04:11]

Done.

+  ~NightLightTest() override {}
+
+  // ash::test::AshTestBase:
+  void SetUp() override {
+    test::AshTestBase::SetUp();
+    NightLightController::DisableAnimationsForTests();
+  }
+
+  void CreateTestUserSessions() {
+    GetSessionControllerClient()->Reset();
+    GetSessionControllerClient()->AddUserSession(user_1_email);
+    GetSessionControllerClient()->AddUserSession(user_2_email);
+  }
+
+  void SwitchActiveUser(const std::string& email) {
+    GetSessionControllerClient()->SwitchActiveUser(
+        AccountId::FromUserEmail(email));
+  }
+
+  void InjectTestPrefService(PrefService* pref_service) {
+    ash_test_helper()->test_shell_delegate()->set_active_user_pref_service(
+        pref_service);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NightLightTest);

[James Cook, 2017/05/05 17:12:31]

#include base/macros.h

[afakhry, 2017/05/05 20:04:11]

Done.

+};
+
+// Tests toggling NightLight on /off and makes sure the observer is updated and

[James Cook, 2017/05/05 17:12:32]

super nit: "on/off" or "on / off"

[afakhry, 2017/05/05 20:04:11]

My bad. Done.

+// the layer temperatures are modified.

[James Cook, 2017/05/05 17:12:32]

Thanks for documenting what you are testing.

[afakhry, 2017/05/05 20:04:11]

Acknowledged.

+TEST_F(NightLightTest, TestToggle) {
+  TestObserver observer;
+  const bool initial_enabled = GetController()->enabled();

[James Cook, 2017/05/05 17:12:32]

nit: I don't think you need this variable, just inline the call

[afakhry, 2017/05/05 20:04:11]

I was planning to use it, but I changed my mind and forgot to remove it. Thanks!

Done.

+  ASSERT_FALSE(initial_enabled);
+  EXPECT_TRUE(TestLayersTemperature(0.0f));
+  GetController()->Toggle();

[James Cook, 2017/05/05 17:12:32]

optional: Just cache a NightLightController* controller at start of test

[afakhry, 2017/05/05 20:04:11]

Done.

+  EXPECT_TRUE(GetController()->enabled());
+  EXPECT_TRUE(observer.status());
+  EXPECT_TRUE(TestLayersTemperature(GetController()->color_temperature()));
+  GetController()->Toggle();
+  EXPECT_FALSE(GetController()->enabled());
+  EXPECT_FALSE(observer.status());
+  EXPECT_TRUE(TestLayersTemperature(0.0f));
+}
+
+// Tests that switching users retrieves NightLight settings for the active
+// user's prefs.
+TEST_F(NightLightTest, TestUserSwitchAndSettingsPersistence) {
+  if (Shell::GetAshConfig() == Config::MASH) {
+    // Doesn't work on mash.

[James Cook, 2017/05/05 17:12:32]

Can you clarify why? (Feature doesn't work at all due to layer differences, or
test doesn't work due to session state issues, or mash doesn't handle user
switching properly...)

[afakhry, 2017/05/05 20:04:11]

I think it's because of user switching doesn't work on mash:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...

But I didn't write a comment here as I was waiting to see Xiyuan today to
confirm first, but he's not here.

+    return;
+  }
+
+  CreateTestUserSessions();
+  TestingPrefServiceSimple user_1_pref_service;
+  TestingPrefServiceSimple user_2_pref_service;
+  NightLightController::RegisterPrefs(user_1_pref_service.registry());
+  NightLightController::RegisterPrefs(user_2_pref_service.registry());
+
+  // Simulate user 1 login.
+  InjectTestPrefService(&user_1_pref_service);
+  SwitchActiveUser(user_1_email);
+  GetController()->SetEnabled(true);
+  EXPECT_TRUE(GetController()->enabled());
+  EXPECT_TRUE(TestLayersTemperature(GetController()->color_temperature()));
+
+  // Switch to user 2, and expect NightLight to be disabled.
+  InjectTestPrefService(&user_2_pref_service);
+  SwitchActiveUser(user_2_email);
+  EXPECT_FALSE(GetController()->enabled());
+
+  // Switch back to user 1, to find NightLight is still enabled.
+  InjectTestPrefService(&user_1_pref_service);
+  SwitchActiveUser(user_1_email);
+  EXPECT_TRUE(GetController()->enabled());
+}

[James Cook, 2017/05/05 17:12:32]

Nice tests. Easy to read. Good balance between clarifying comments and just
letting the test expectations speak for themselves.

[afakhry, 2017/05/05 20:04:11]

Thank you very much!

+
+}  // namespace
+
+}  // namespace ash