[CrOS Tether] Create MessageWrapper, a wrapper class for proto messages sent for tethering. This cl…

chromeos/components/tether/message_wrapper_unittest.cc

Index: chromeos/components/tether/message_wrapper_unittest.cc
diff --git a/chromeos/components/tether/message_wrapper_unittest.cc b/chromeos/components/tether/message_wrapper_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b74a06f08abc9c7d2e4a1bc2ad7a2d8745ba87dc
--- /dev/null
+++ b/chromeos/components/tether/message_wrapper_unittest.cc
@@ -0,0 +1,155 @@
+// 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 "chromeos/components/tether/message_wrapper.h"
+
+#include <sstream>
+
+#include "base/base64url.h"
+#include "base/macros.h"
+#include "chromeos/components/tether/proto/tether.pb.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+
+namespace tether {
+
+namespace {
+
+DeviceStatus CreateFakeDeviceStatus() {
+  WifiStatus wifi_status;
+  wifi_status.set_status_code(
+      WifiStatus_StatusCode::WifiStatus_StatusCode_CONNECTED);
+  wifi_status.set_ssid("Google A");
+
+  DeviceStatus device_status;
+  device_status.set_battery_percentage(75);
+  device_status.set_cell_provider("Google Fi");
+  device_status.set_connection_strength(4);
+  device_status.mutable_wifi_status()->CopyFrom(wifi_status);
+
+  return device_status;
+}
+
+void VerifyProtoConversion(const google::protobuf::MessageLite* proto,
+                           const MessageWrapper& wrapper,
+                           const MessageType& expected_message_type) {
+  std::string raw_message = wrapper.ToRawMessage();
+  EXPECT_TRUE(raw_message.length() > 0);
+
+  std::unique_ptr<MessageWrapper> wrapper_from_raw_message =
+      MessageWrapper::FromRawMessage(raw_message);
+  EXPECT_TRUE(wrapper_from_raw_message);
+  EXPECT_EQ(expected_message_type, wrapper_from_raw_message->GetMessageType());
+  EXPECT_EQ(proto->SerializeAsString(),
+            wrapper_from_raw_message->GetProto()->SerializeAsString());
+}
+
+}  // namespace
+
+class MessageWrapperTest : public testing::Test {
+ protected:
+  MessageWrapperTest() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MessageWrapperTest);
+};
+
+TEST_F(MessageWrapperTest, TestToAndFromRawMessage_ConnectTetheringRequest) {
+  ConnectTetheringRequest request;
+
+  MessageWrapper wrapper(request);

[Ryan Hansberry, 2017/02/17 02:29:07]

Constructing a MessageWrapper is common to all of these tests. Can you
encapsulate that in VerifyProtoConversion? (would also reduce the argument
count)

[Kyle Horimoto, 2017/02/17 20:05:04]

It's not a good idea to do that. MessageWrapper's constructors only take
specific message types (e.g., ConnectTetheringRequest) as parameters. If we
created the MessageWrapper in VerifyProtoConversion(), we would have to cast the
google::protobuf::MessageLite* parameter back to the original pointer type
before passing it to MessageWrapper's constructor. This would result in a switch
statement on the MessageType casting the pointer to a different type based on
each possible MessageType. The code is much easier to read as written.

+  VerifyProtoConversion(&request, wrapper,
+                        MessageType::CONNECT_TETHERING_REQUEST);
+}
+
+TEST_F(MessageWrapperTest, TestToAndFromRawMessage_ConnectTetheringResponse) {
+  ConnectTetheringResponse response;
+  response.set_ssid("Instant Tethering 123456");
+  response.set_password("password");
+  response.set_response_code(ConnectTetheringResponse_ResponseCode::
+                                 ConnectTetheringResponse_ResponseCode_SUCCESS);
+  response.mutable_device_status()->CopyFrom(CreateFakeDeviceStatus());
+
+  MessageWrapper wrapper(response);
+  VerifyProtoConversion(&response, wrapper,
+                        MessageType::CONNECT_TETHERING_RESPONSE);
+}
+
+TEST_F(MessageWrapperTest, TestToAndFromRawMessage_DisconnectTetheringRequest) {
+  DisconnectTetheringRequest request;
+
+  MessageWrapper wrapper(request);
+  VerifyProtoConversion(&request, wrapper,
+                        MessageType::DISCONNECT_TETHERING_REQUEST);
+}
+
+TEST_F(MessageWrapperTest, TestToAndFromRawMessage_KeepAliveTickle) {
+  KeepAliveTickle tickle;
+
+  MessageWrapper wrapper(tickle);
+  VerifyProtoConversion(&tickle, wrapper, MessageType::KEEP_ALIVE_TICKLE);
+}
+
+TEST_F(MessageWrapperTest, TestToAndFromRawMessage_TetherAvailabilityRequest) {
+  TetherAvailabilityRequest request;
+
+  MessageWrapper wrapper(request);
+  VerifyProtoConversion(&request, wrapper,
+                        MessageType::TETHER_AVAILABILITY_REQUEST);
+}
+
+TEST_F(MessageWrapperTest, TestToAndFromRawMessage_TetherAvailabilityResponse) {
+  TetherAvailabilityResponse response;
+  response.set_response_code(
+      TetherAvailabilityResponse_ResponseCode::
+          TetherAvailabilityResponse_ResponseCode_TETHER_AVAILABLE);
+  response.mutable_device_status()->CopyFrom(CreateFakeDeviceStatus());
+
+  MessageWrapper wrapper(response);
+  VerifyProtoConversion(&response, wrapper,
+                        MessageType::TETHER_AVAILABILITY_RESPONSE);
+}
+
+TEST_F(MessageWrapperTest, TestHandleInvalidJson) {
+  EXPECT_FALSE(MessageWrapper::FromRawMessage("not JSON"));
+}
+
+TEST_F(MessageWrapperTest, TestHandleJsonWithoutType) {
+  ConnectTetheringRequest request;
+  std::string encoded_message;
+  base::Base64UrlEncode(request.SerializeAsString(),
+                        base::Base64UrlEncodePolicy::INCLUDE_PADDING,
+                        &encoded_message);
+
+  std::stringstream ss;
+  ss << "{data: \"" << encoded_message << "\"}";

[Ryan Hansberry, 2017/02/17 02:29:07]

I think JSON keys are wrapped in quotes too -- please check.

[Kyle Horimoto, 2017/02/17 20:05:04]

Done.

+
+  EXPECT_FALSE(MessageWrapper::FromRawMessage(ss.str()));
+}
+
+TEST_F(MessageWrapperTest, TestHandleJsonWithoutData) {
+  EXPECT_FALSE(MessageWrapper::FromRawMessage("{type: 1}"));
+}
+
+TEST_F(MessageWrapperTest, TestHandleJsonWithoutTypeOrData) {
+  EXPECT_FALSE(MessageWrapper::FromRawMessage("{}"));
+}
+
+TEST_F(MessageWrapperTest, TestHandleDataNotEncodedWithBase64) {
+  ConnectTetheringRequest request;
+
+  std::stringstream ss;
+  ss << "{type: " << static_cast<int>(MessageType::CONNECT_TETHERING_REQUEST)
+     << ", data: \""
+     << request.SerializeAsString()  // Do not convert to base-64.
+     << "\"}";
+
+  EXPECT_FALSE(MessageWrapper::FromRawMessage(ss.str()));
+}
+

[Ryan Hansberry, 2017/02/17 02:29:07]

It's a PITA, but do you see any value adding a sanity check test where you craft
a valid JSON dict string (e.g. {"type": 1, "data": "<actual b54 encoded data>"})
and ensure that it's parsed correctly?

[Kyle Horimoto, 2017/02/17 20:05:04]

Done.

+}  // namespace tether

[Ryan Hansberry, 2017/02/17 02:29:07]

Please add test that makes sure this works when the JSON dict has an extra key
besides "type" or "data"

[Kyle Horimoto, 2017/02/17 20:05:04]

Done.

+
+}  // namespace cryptauth