Add a row in the network tray to inform users to turn Bluetooth on to enable Tether.

ash/system/tray/tray_info_label_unittest.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/system/tray/tray_info_label.h"
+#include "ash/test/ash_test_base.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ash {
+namespace test {
+
+namespace {
+
+const int clickableMessageId = 1;

[Kyle Horimoto, 2017/07/13 23:15:36]

nit: Constants are prefixed with a k: (e.g., kClickableMessageId). Same below.

(But I think it will be better to use the suggestions I had below instead.)

[lesliewatkins, 2017/07/14 06:41:52]

Acknowledged.

I really would prefer to keep the constants-- I think they're easier to read and
more accurately test the scenario where this class is being used. I also think
that they fully test everything in a simpler way.

[Kyle Horimoto, 2017/07/14 17:54:22]

The reason that I do not like these is because they are misleading when we don't
have a delegate. For example, your test currently has these lines:

label_->Update(kClickableMessageId);
VerifyClickability(false);

This makes the test hard to understand since, presumably, if you set a clickable
message ID, your label should be clickable.

Since the clickability of a message ID depends on the delegate's implementation,
it does not makes sense to make constants which are agnostic to the delegate.

[lesliewatkins, 2017/07/14 20:43:16]

Alrighty, done.

+const int nonClickableMessageId = 0;
+
+class TestClickEvent : public ui::Event {
+ public:
+  TestClickEvent() : ui::Event(ui::ET_MOUSE_PRESSED, base::TimeTicks(), 0) {}
+};
+
+class TestDelegate : public TrayInfoLabel::Delegate {
+ public:
+  TestDelegate() : clicked_message_ids_(std::vector<int>()) {}
+
+  const std::vector<int>& clicked_message_ids() { return clicked_message_ids_; }
+
+  void OnLabelClicked(int message_id) override {
+    clicked_message_ids_.push_back(message_id);
+  }
+
+  bool IsLabelClickable(int message_id) override {
+    return clickableMessageId == message_id;
+  }
+
+ private:
+  std::vector<int> clicked_message_ids_;
+};
+
+}  // namespace
+
+class TrayInfoLabelTest : public AshTestBase {
+ public:
+  std::unique_ptr<TrayInfoLabel> label_;

[Kyle Horimoto, 2017/07/13 23:15:36]

nit: Move these to the bottom of the test class - functions should appear first.
Also, make them protected.

[lesliewatkins, 2017/07/14 06:41:52]

Done.

+  std::unique_ptr<TestDelegate> delegate_;
+
+  void CreateLabel(bool use_delegate, int message_id) {
+    if (use_delegate)
+      delegate_ = base::MakeUnique<TestDelegate>() else delegate_ = nullptr;

[Kyle Horimoto, 2017/07/13 23:15:36]

Instead of setting delegate_ to nullptr in the else case, please add a
TearDown() function which resets the pointer. That way, we can just assume that
the delegate starts off as null.

Two nits:
(1) Instead of setting a std::unique_ptr to nullptr, just call reset().
(2) It's difficult to read when you put the "else" condition on the same line as
the "if". You should always put the "else" on a new line.

How TearDown() should look:

void TearDown() override {
  label_.reset();
  delegate_.reset();
}

[lesliewatkins, 2017/07/14 06:41:52]

Done.

+
+    label_ = base::MakeUnique<TrayInfoLabel>(delegate_.get(), message_id);
+  }
+
+  void UpdateLabel(int message_id) { label_->Update(message_id); }

[Kyle Horimoto, 2017/07/13 23:15:37]

No need for this function - Update() is already a public function on
TrayInfoLabel, so you can just call the function directly.

[lesliewatkins, 2017/07/14 06:41:52]

Done.

+
+  void ClickOnLabel() { label_->PerformAction(TestClickEvent()); }

[Kyle Horimoto, 2017/07/13 23:15:36]

Let's also verify that the correct bool is returned by PerformAction().

To do so, take a click_expected_to_be_handled bool as a parameter to
ClickOnLabel(), then verify that the value is what we expect:

void ClickOnLabel(bool click_expected_to_be_handled) {
  bool was_handled = label_->PerformAction(TestClickEvent());
  EXPECT_EQ(click_expected_to_be_handled, was_handled);
}

[lesliewatkins, 2017/07/14 06:41:52]

Done.

+
+  VerifyMessageId(int expected_message_id) {

[Kyle Horimoto, 2017/07/13 23:15:37]

No need for this function either. In general, you should only test the public
API of the class under test as well as whether the class under test interacted
with other objects which have been injected into the class under test. From the
perspective of a test, we shouldn't care about what the internal details of the
class look like as long as it works.

Imagine, for example, that someone edits TrayInfoLabel later and decides that
the message ID should be stored internally as a uint64_t instead of an int. This
person would only be changing internal implementation details of the class, and
none of the public-facing API should change how it functions. In this case, we
would want the unit tests for this class to continue to work as expected,
without any modification. If you add tests which use implementation details
(i.e., private/protected fields) of the class, the test becomes more brittle and
must be updated when the implementation changes, even if the functionality of
the class does not change.

[lesliewatkins, 2017/07/14 06:41:52]

Done.

+    EXPECT_EQ(expected_message_id, label_->message_id_);
+  }
+
+  void VerifyClickability(bool expected_clickable) {
+    EXPECT_EQ(expected_clickable, label_->IsClickable());
+  }
+
+  void VerifyClicks(const std::vector<int> expected_clicked_message_ids) {
+    if (!delegate_) {
+      EXPECT_TRUE(expected_clicked_message_ids.empty());
+      return;
+    }
+
+    EXPECT_EQ(expected_clicked_message_ids, delegate_->clicked_message_ids());
+  }
+
+  void VerifyNoClicks() { VerifyClicks(std::vector<int>()); }
+};
+
+TEST_F(TrayInfoLabelTest, NoDelegate) {
+  CreateLabel(false, nonClickableMessageId);

[Kyle Horimoto, 2017/07/13 23:15:36]

nit: /* use_delegate */

[lesliewatkins, 2017/07/14 06:41:52]

Done.

+
+  VerifyClickability(false);

[Kyle Horimoto, 2017/07/13 23:15:37]

After this, update the label to another message ID and verify that it is still
not clickable.

In my comment on the last patchset, I suggested adding a clickable_message_ids
set which you could set on a per-test basis. This makes it easy to determine
which is clickable and which is not, instead of using clickable/non-clickable
constants. If you went that route, this test could read like:

TEST_F(TrayInfoLabelTest, NoDelegate) {
  CreateLabel(false /* use_delegate */, 1 /* message_id */);
  VerifyClickability(false);

  label_->Update(2 /* message_id *);
  VerifyClickability(false);

  label_->Update(3 /* message_id *);
  VerifyClickability(false);
}

[lesliewatkins, 2017/07/14 06:41:52]

Done.

+}
+
+TEST_F(TrayInfoLabelTest, NonClickableDelegate) {

[Kyle Horimoto, 2017/07/13 23:15:36]

Instead of having a delegate which is entirely clickable vs. entirely
non-clickable and no updates vs. one update, let's write one test which combines
these:

TEST_F(TrayInfoLabelTest, TestUpdate) {
  // Message IDs 1 and 2 are clickable; all others are not.
  delegate_->AddClickableMessageId(1);
  delegate_->AddClickableMessageId(2);

  CreateLabel(true /* use_delegate */, 1 /* message_id */);
  VerifyClickability(true);

  label_->Update(2 /* message_id *);
  VerifyClickability(true);

  label_->Update(3 /* message_id *);
  VerifyClickability(false);

  label_->Update(4 /* message_id *);
  VerifyClickability(false);
}

[lesliewatkins, 2017/07/14 06:41:52]

Done.

+  CreateLabel(true, nonClickableMessageId);
+
+  VerifyClickability(false);
+}
+
+TEST_F(TrayInfoLabelTest, ClickableDelegate) {
+  CreateLabel(true, clickableMessageId);
+
+  VerifyClickability(true);
+}
+
+TEST_F(TrayInfoLabelTest, PerformAction) {

[Kyle Horimoto, 2017/07/13 23:15:36]

To test performing actions, verify that you can click multiple times on
different message IDs, and verify that clicking when the label is not clickable
does not result in a registered click:

TEST_F(TrayInfoLabelTest, TestClicks) {
  // Message IDs 1 and 2 are clickable; all others are not.
  delegate_->AddClickableMessageId(1);
  delegate_->AddClickableMessageId(2);

  CreateLabel(true /* use_delegate */, 1 /* message_id */);
  VerifyClickability(true);

  // Click once.
  ClickOnLabel(true /* click_expected_to_be_handled */);
  VerifyClicks(std::vector<int> {1});

  // Click again.
  ClickOnLabel(true /* click_expected_to_be_handled */);
  VerifyClicks(std::vector<int> {1, 1});

  // Update to another clickable message ID and click again.
  label_->Update(2 /* message_id *);
  VerifyClickability(true);
  ClickOnLabel(true /* click_expected_to_be_handled */);
  VerifyClicks(std::vector<int> {1, 1, 2});

  // Update to a non-clickable message ID.
  label_->Update(3 /* message_id *);
  VerifyClickability(false);

  // Click on the label; the delegate should not have received a
  // click, so the verified clicks should remain the same.
  ClickOnLabel(false /* click_expected_to_be_handled */);
  VerifyClicks(std::vector<int> {1, 1, 2});
}

[lesliewatkins, 2017/07/14 06:41:52]

Done.

[Kyle Horimoto, 2017/07/14 17:54:21]

Hey Leslie, doesn't look like this was done. Your test only simulates one click
on one clickable ID and one click on one non-clickable ID. Please include
multiple clicks on multiple clickable and multiple non-clickable IDs.

[lesliewatkins, 2017/07/14 20:43:16]

Okay, I added some more clicks.

+  CreateLabel(true, clickableMessageId);
+  VerifyNoClicks();
+
+  ClickOnLabel();
+
+  VerifyClicks(std::vector<int>({clickableMessageId}));
+}
+
+TEST_F(TrayInfoLabelTest, Update) {
+  CreateLabel(true, nonClickableMessageId);
+  VerifyMessageId(nonClickableMessageId);
+  VerifyClickability(false);
+
+  UpdateLabel(clickableMessageId);
+  VerifyMessageId(clickableMessageId);
+  VerifyClickability(true);
+}
+
+}  // namespace test
+}  // namespace ash