Enhanced and refactored the AttestationFlow interface.

chromeos/attestation/attestation_flow_unittest.cc

Index: chromeos/attestation/attestation_flow_unittest.cc
diff --git a/chromeos/attestation/attestation_flow_unittest.cc b/chromeos/attestation/attestation_flow_unittest.cc
index 30ea3cc9750845fcd198ee49524ed6e06ca848cd..56438895017d6f4c3912d78101f0e72ad9a0eb1c 100644
--- a/chromeos/attestation/attestation_flow_unittest.cc
+++ b/chromeos/attestation/attestation_flow_unittest.cc
@@ -13,6 +13,7 @@
 
 using testing::_;
 using testing::Invoke;
+using testing::NiceMock;

[Mattias Nissler (ping if slow), 2013/04/24 12:57:53]

unused?

[dkrahn, 2013/04/25 01:06:52]

Done.

 using testing::Sequence;
 using testing::StrictMock;
 using testing::WithArgs;
@@ -41,6 +42,20 @@ void AsyncCallbackFalse(cryptohome::AsyncMethodCaller::Callback callback) {
   callback.Run(false, cryptohome::MOUNT_ERROR_NONE);
 }
 
+class FakeDBusData {
+ public:
+  explicit FakeDBusData(const std::string& data) : data_(data) {}
+
+  void operator() (const CryptohomeClient::DataMethodCallback& callback) {
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, DBUS_METHOD_CALL_SUCCESS, true, data_));
+  }
+
+ private:
+  std::string data_;
+};
+
 }  // namespace
 
 class AttestationFlowTest : public testing::Test {
@@ -83,9 +98,8 @@ TEST_F(AttestationFlowTest, GetCertificate) {
       .Times(1)
       .InSequence(flow_order);
 
-  int options = CryptohomeClient::INCLUDE_DEVICE_STATE;
   EXPECT_CALL(async_caller,
-              AsyncTpmAttestationCreateCertRequest(options, _))
+              AsyncTpmAttestationCreateCertRequest(INCLUDE_DEVICE_STATE, _))
       .Times(1)
       .InSequence(flow_order);
 
@@ -99,8 +113,8 @@ TEST_F(AttestationFlowTest, GetCertificate) {
   fake_cert_response += "_response";
   EXPECT_CALL(async_caller,
               AsyncTpmAttestationFinishCertRequest(fake_cert_response,
-                                                   CryptohomeClient::USER_KEY,
-                                                   "test",
+                                                   USER_KEY,
+                                                   kEnterpriseUserKey,
                                                    _))
       .Times(1)
       .InSequence(flow_order);
@@ -117,7 +131,7 @@ TEST_F(AttestationFlowTest, GetCertificate) {
 
   scoped_ptr<ServerProxy> proxy_interface(proxy.release());
   AttestationFlow flow(&async_caller, &client, proxy_interface.Pass());
-  flow.GetCertificate("test", mock_callback);
+  flow.GetCertificate(ENTERPRISE_USER_CERTIFICATE, true, mock_callback);
   Run();
 }
 
@@ -143,7 +157,7 @@ TEST_F(AttestationFlowTest, GetCertificate_NoEK) {
 
   scoped_ptr<ServerProxy> proxy_interface(proxy.release());
   AttestationFlow flow(&async_caller, &client, proxy_interface.Pass());
-  flow.GetCertificate("test", mock_callback);
+  flow.GetCertificate(ENTERPRISE_USER_CERTIFICATE, true, mock_callback);
   Run();
 }
 
@@ -172,7 +186,7 @@ TEST_F(AttestationFlowTest, GetCertificate_EKRejected) {
 
   scoped_ptr<ServerProxy> proxy_interface(proxy.release());
   AttestationFlow flow(&async_caller, &client, proxy_interface.Pass());
-  flow.GetCertificate("test", mock_callback);
+  flow.GetCertificate(ENTERPRISE_USER_CERTIFICATE, true, mock_callback);
   Run();
 }
 
@@ -205,15 +219,15 @@ TEST_F(AttestationFlowTest, GetCertificate_FailEnroll) {
 
   scoped_ptr<ServerProxy> proxy_interface(proxy.release());
   AttestationFlow flow(&async_caller, &client, proxy_interface.Pass());
-  flow.GetCertificate("test", mock_callback);
+  flow.GetCertificate(ENTERPRISE_USER_CERTIFICATE, true, mock_callback);
   Run();
 }
 
-TEST_F(AttestationFlowTest, GetOwnerCertificateAlreadyEnrolled) {
+TEST_F(AttestationFlowTest, GetMachineCertificateAlreadyEnrolled) {
   StrictMock<cryptohome::MockAsyncMethodCaller> async_caller;
   async_caller.SetUp(true, cryptohome::MOUNT_ERROR_NONE);
-  int options = CryptohomeClient::INCLUDE_DEVICE_STATE |
-                CryptohomeClient::INCLUDE_STABLE_ID;
+  int options = INCLUDE_DEVICE_STATE |
+                INCLUDE_STABLE_ID;
   EXPECT_CALL(async_caller, AsyncTpmAttestationCreateCertRequest(options, _))
       .Times(1);
   std::string fake_cert_response =
@@ -221,8 +235,8 @@ TEST_F(AttestationFlowTest, GetOwnerCertificateAlreadyEnrolled) {
   fake_cert_response += "_response";
   EXPECT_CALL(async_caller,
               AsyncTpmAttestationFinishCertRequest(fake_cert_response,
-                                                   CryptohomeClient::DEVICE_KEY,
-                                                   "attest-ent-machine",
+                                                   DEVICE_KEY,
+                                                   kEnterpriseMachineKey,
                                                    _))
       .Times(1);
 
@@ -246,14 +260,14 @@ TEST_F(AttestationFlowTest, GetOwnerCertificateAlreadyEnrolled) {
 
   scoped_ptr<ServerProxy> proxy_interface(proxy.release());
   AttestationFlow flow(&async_caller, &client, proxy_interface.Pass());
-  flow.GetCertificate("attest-ent-machine", mock_callback);
+  flow.GetCertificate(ENTERPRISE_MACHINE_CERTIFICATE, true, mock_callback);
   Run();
 }
 
 TEST_F(AttestationFlowTest, GetCertificate_FailCreateCertRequest) {
   StrictMock<cryptohome::MockAsyncMethodCaller> async_caller;
   async_caller.SetUp(false, cryptohome::MOUNT_ERROR_NONE);
-  int options = CryptohomeClient::INCLUDE_DEVICE_STATE;
+  int options = INCLUDE_DEVICE_STATE;
   EXPECT_CALL(async_caller,
               AsyncTpmAttestationCreateCertRequest(options, _))
       .Times(1);
@@ -273,14 +287,14 @@ TEST_F(AttestationFlowTest, GetCertificate_FailCreateCertRequest) {
 
   scoped_ptr<ServerProxy> proxy_interface(proxy.release());
   AttestationFlow flow(&async_caller, &client, proxy_interface.Pass());
-  flow.GetCertificate("test", mock_callback);
+  flow.GetCertificate(ENTERPRISE_USER_CERTIFICATE, true, mock_callback);
   Run();
 }
 
 TEST_F(AttestationFlowTest, GetCertificate_CertRequestRejected) {
   StrictMock<cryptohome::MockAsyncMethodCaller> async_caller;
   async_caller.SetUp(true, cryptohome::MOUNT_ERROR_NONE);
-  int options = CryptohomeClient::INCLUDE_DEVICE_STATE;
+  int options = INCLUDE_DEVICE_STATE;
   EXPECT_CALL(async_caller,
               AsyncTpmAttestationCreateCertRequest(options, _))
       .Times(1);
@@ -303,12 +317,12 @@ TEST_F(AttestationFlowTest, GetCertificate_CertRequestRejected) {
 
   scoped_ptr<ServerProxy> proxy_interface(proxy.release());
   AttestationFlow flow(&async_caller, &client, proxy_interface.Pass());
-  flow.GetCertificate("test", mock_callback);
+  flow.GetCertificate(ENTERPRISE_USER_CERTIFICATE, true, mock_callback);
   Run();
 }
 
 TEST_F(AttestationFlowTest, GetCertificate_FailIsEnrolled) {
-  // We're not expecting any server calls in this case; StrictMock will verify.
+  // We're not expecting any async calls in this case; StrictMock will verify.
   StrictMock<cryptohome::MockAsyncMethodCaller> async_caller;
 
   chromeos::MockCryptohomeClient client;
@@ -326,7 +340,79 @@ TEST_F(AttestationFlowTest, GetCertificate_FailIsEnrolled) {
 
   scoped_ptr<ServerProxy> proxy_interface(proxy.release());
   AttestationFlow flow(&async_caller, &client, proxy_interface.Pass());
-  flow.GetCertificate("test", mock_callback);
+  flow.GetCertificate(ENTERPRISE_USER_CERTIFICATE, true, mock_callback);
+  Run();
+}
+
+TEST_F(AttestationFlowTest, GetCertificate_CheckExisting) {
+  StrictMock<cryptohome::MockAsyncMethodCaller> async_caller;
+  async_caller.SetUp(true, cryptohome::MOUNT_ERROR_NONE);
+  int options = INCLUDE_DEVICE_STATE;
+  EXPECT_CALL(async_caller, AsyncTpmAttestationCreateCertRequest(options, _))
+      .Times(1);
+  std::string fake_cert_response =
+      cryptohome::MockAsyncMethodCaller::kFakeAttestationCertRequest;
+  fake_cert_response += "_response";
+  EXPECT_CALL(async_caller,
+              AsyncTpmAttestationFinishCertRequest(fake_cert_response,
+                                                   USER_KEY,
+                                                   kEnterpriseUserKey,
+                                                   _))
+      .Times(1);
+
+  chromeos::MockCryptohomeClient client;
+  EXPECT_CALL(client, TpmAttestationIsEnrolled(_))
+      .WillRepeatedly(Invoke(DBusCallbackTrue));
+  EXPECT_CALL(client,
+              TpmAttestationDoesKeyExist(USER_KEY, kEnterpriseUserKey, _))
+      .WillRepeatedly(WithArgs<2>(Invoke(DBusCallbackFalse)));
+
+  scoped_ptr<MockServerProxy> proxy(new StrictMock<MockServerProxy>());
+  proxy->DeferToFake(true);
+  EXPECT_CALL(*proxy, SendCertificateRequest(
+      cryptohome::MockAsyncMethodCaller::kFakeAttestationCertRequest,
+      _)).Times(1);
+
+  StrictMock<MockObserver> observer;
+  EXPECT_CALL(observer, MockCertificateCallback(
+      true,
+      cryptohome::MockAsyncMethodCaller::kFakeAttestationCert)).Times(1);
+  AttestationFlow::CertificateCallback mock_callback = base::Bind(
+      &MockObserver::MockCertificateCallback,
+      base::Unretained(&observer));
+
+  scoped_ptr<ServerProxy> proxy_interface(proxy.release());
+  AttestationFlow flow(&async_caller, &client, proxy_interface.Pass());
+  flow.GetCertificate(ENTERPRISE_USER_CERTIFICATE, false, mock_callback);
+  Run();
+}
+
+TEST_F(AttestationFlowTest, GetCertificate_AlreadyExists) {
+  // We're not expecting any async calls in this case; StrictMock will verify.
+  StrictMock<cryptohome::MockAsyncMethodCaller> async_caller;
+
+  chromeos::MockCryptohomeClient client;
+  EXPECT_CALL(client, TpmAttestationIsEnrolled(_))
+      .WillRepeatedly(Invoke(DBusCallbackTrue));
+  EXPECT_CALL(client,
+              TpmAttestationDoesKeyExist(USER_KEY, kEnterpriseUserKey, _))
+      .WillRepeatedly(WithArgs<2>(Invoke(DBusCallbackTrue)));
+  EXPECT_CALL(client,
+              TpmAttestationGetCertificate(USER_KEY, kEnterpriseUserKey, _))
+      .WillRepeatedly(WithArgs<2>(Invoke(FakeDBusData("fake_cert"))));
+
+  // We're not expecting any server calls in this case; StrictMock will verify.
+  scoped_ptr<MockServerProxy> proxy(new StrictMock<MockServerProxy>());
+
+  StrictMock<MockObserver> observer;
+  EXPECT_CALL(observer, MockCertificateCallback(true, "fake_cert")).Times(1);
+  AttestationFlow::CertificateCallback mock_callback = base::Bind(
+      &MockObserver::MockCertificateCallback,
+      base::Unretained(&observer));
+
+  scoped_ptr<ServerProxy> proxy_interface(proxy.release());
+  AttestationFlow flow(&async_caller, &client, proxy_interface.Pass());
+  flow.GetCertificate(ENTERPRISE_USER_CERTIFICATE, false, mock_callback);
   Run();

[Mattias Nissler (ping if slow), 2013/04/24 12:57:53]

Ugh, even more hard-to-read gmock setup. To repeat: I'm with the powers that be
in considering this gmock abuse (see recent thread on blink-dev:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/q9cq3KHZesc/discus...),
and my advice would be to use fakes.

[dkrahn, 2013/04/25 01:06:52]

For testing this flow I believe I have a legitimate case for using mocks.  For
some classes I want a combination of mock / fake.  Let me break it apart:

AsyncMethodCaller - I'm reusing an existing fake (note SetUp() method) which
uses gmock.  If I used a pure fake here I would lose test coverage because I set
a few expectations on the methods (while at the same time deferring to the fake
to implement).

CryptohomeClient - Here I'm abusing gmock to create a simple fake -- until
FakeCryptohomeClient lands this is the simplest solution IMO.

ServerProxy - Again I need a mock / fake combination and am using gmock to defer
to a fake (DeferToFake()) and set expectations at the same time.

MockObserver - This is strictly a mock.

Apart from the CryptohomeClient discussion I think the approach here is
reasonable given what I'm trying to test and that this is a pure unit test.  I
am loath to give up the coverage that mocks currently give me.

[Mattias Nissler (ping if slow), 2013/04/25 12:05:26]

Yes, these seem reasonable.
Fair.
Why is the mocking part needed here? A pure fake implementation gives you the
same kind of guarantees if you put code verifying the state of the fake after
the test. The Times(1) specification is more difficult to verify obviously, if
you're concerned about that you could just keep a counter and verify its value.
A function that just records the callback arguments and proper EXPECT_EQ() after
the test would be easier to read IMHO.
 
It's not about giving up coverage. It's about people reading your code being
able to find out what you're verifying in a reasonable amount of time. The
pattern complicated-setup -> run-code-under-test -> automatic-verification is
inherently harder to follow than setup-dependency-state -> run-code-under-test
-> verify-state IMO.

The other thing you seem to be worried about in terms of coverage are your
narrow expectations for parameters passed to the mocks in calls and the sequence
of the calls. If it is important to validate code behavior w.r.t. this, then
you're essentially saying you want to verify that some data flows in the right
ways through the system. In this case, you create an enroll request, send it to
the server, receive a response etc. and data gets passed on from one step to
another. With fakes that behave similar to the real thing, you could as well
just run AttestationFlow without any narrow expectations. Once the flow
finishes, just look at the state of the fakes. If the result is as expected, the
data flow must have been correct and the code must have called the fakes in the
right order (otherwise the request payload wouldn't match the request the server
fake saw etc.).

 }