Generalize ScopedXPCUpdateServiceMock to ScopedXPCServiceMock.

It now takes an XPC protocol as a parameter and mocks the service
specified. Some type safety is lost in the process, since I had to
convert id<P> to raw id in a few spots; I spend a while trying to
figure out how to do this with templates and concluded that I probably
couldn't (because C++ does not understand an Objective-C protocol,
since it is completely outside the C++ type system), and figured the unchecked type is tolerable in test code.

Bug: 1055876
Change-Id: I2fe5684b34b64245f6d9fe7716c9bdf7cd97c76e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2231703Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Adam Norberg <norberg@google.com>
Cr-Commit-Position: refs/heads/master@{#776576}

chrome/updater/mac/scoped_xpc_service_mock.h

@@ -15,19 +15,18 @@
 #include "base/test/task_environment.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #import "chrome/updater/mac/xpc_service_names.h"
-#import "chrome/updater/server/mac/service_protocol.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/gtest_mac.h"
 #import "third_party/ocmock/OCMock/OCMock.h"
 #include "third_party/ocmock/gtest_support.h"
 
 namespace updater {
-// ScopedXPCUpdateServiceMock sets up mocks for the XPC Updater Service and
+// ScopedXPCServiceMock sets up mocks for an arbitrary XPC service and
 // provides access to those mocks. Mocks are removed when the object is
 // deallocated. Only one of these objects can exist at a time, although
 // it is not a singleton, since creating and destroying the mock manager
 // is part of its behavior.
-class ScopedXPCUpdateServiceMock {
+class ScopedXPCServiceMock {
  public:
   // RemoteObjectMockRecord represents a single call to a mock NSXPCConnection's
   // remoteObjectProxyWithErrorHandler: or remoteObjectProxy method.
@@ -38,7 +37,7 @@ class ScopedXPCUpdateServiceMock {
   struct RemoteObjectMockRecord {
     // The mock object that will be served. Use this to configure behaviors
     // and expectations for the test.
-    const base::scoped_nsprotocol<id<CRUUpdateChecking>> mock_object;
+    const base::scoped_nsprotocol<id> mock_object;
 
     // The error handler provided when the remote object was requested by the
     // code under test. Will not be populated before that request is issued.
@@ -47,8 +46,7 @@ class ScopedXPCUpdateServiceMock {
     base::Optional<base::mac::ScopedBlock<void (^)(NSError*)>>
         xpc_error_handler;
 
-    RemoteObjectMockRecord(
-        base::scoped_nsprotocol<id<CRUUpdateChecking>> mock_ptr);
+    explicit RemoteObjectMockRecord(base::scoped_nsprotocol<id> mock_ptr);
     ~RemoteObjectMockRecord();
   };
 
@@ -58,11 +56,11 @@ class ScopedXPCUpdateServiceMock {
    public:
     // Constructs a ConnectionMockRecord to be served in the specified order.
     // This constructor is not intended for use outside of
-    // ScopedXPCUpdateServiceMock. It produces a mock connection that is ready
+    // ScopedXPCServiceMock. It produces a mock connection that is ready
     // for use, but has no associated mock remote objects.
-    // It requires a pointer to its enclosing ScopedXPCUpdateServiceMock to
+    // It requires a pointer to its enclosing ScopedXPCServiceMock to
     // maintain correct behavior of mocked alloc operations.
-    ConnectionMockRecord(ScopedXPCUpdateServiceMock* mock_driver, size_t index);
+    ConnectionMockRecord(ScopedXPCServiceMock* mock_driver, size_t index);
     ~ConnectionMockRecord();
 
     ConnectionMockRecord(const ConnectionMockRecord& other) = delete;
@@ -107,7 +105,8 @@ class ScopedXPCUpdateServiceMock {
     void Verify() const;
 
    private:
-    // The index of |this| in ScopedXPCUpdateServiceMock::mocked_connections_.
+    Protocol* service_protocol_;  // Copied from mock_driver
+    // The index of |this| in ScopedXPCServiceMock::mocked_connections_.
     // Used in verification failure messages.
     const size_t index_;
     base::scoped_nsobject<id> mock_connection_;  // mock NSXPCConnection
@@ -115,14 +114,16 @@ class ScopedXPCUpdateServiceMock {
     size_t next_mock_to_vend_ = 0;
   };  // class ConnectionMockRecord
 
-  ScopedXPCUpdateServiceMock();
-  ~ScopedXPCUpdateServiceMock();
+  // Constructs a ScopedXPCServiceMock, stubbing out NSXPCConnection to create
+  // mocks for connections to an XPC service fitting the provided protocol.
+  // The mocks are cleaned up when the constructed object is destructed.
+  explicit ScopedXPCServiceMock(Protocol* service_protocol);
+  ~ScopedXPCServiceMock();
 
-  ScopedXPCUpdateServiceMock(const ScopedXPCUpdateServiceMock&) = delete;
-  ScopedXPCUpdateServiceMock& operator=(const ScopedXPCUpdateServiceMock&) =
-      delete;
-  ScopedXPCUpdateServiceMock(ScopedXPCUpdateServiceMock&&) = delete;
-  ScopedXPCUpdateServiceMock& operator=(ScopedXPCUpdateServiceMock&&) = delete;
+  ScopedXPCServiceMock(const ScopedXPCServiceMock&) = delete;
+  ScopedXPCServiceMock& operator=(const ScopedXPCServiceMock&) = delete;
+  ScopedXPCServiceMock(ScopedXPCServiceMock&&) = delete;
+  ScopedXPCServiceMock& operator=(ScopedXPCServiceMock&&) = delete;
 
   // Prepares another mock connection to be served on a consecutive expected
   // call to +[NSXPCConnection alloc]. Returns the record for the new mock.
@@ -153,12 +154,15 @@ class ScopedXPCUpdateServiceMock {
   // have been prepared, this prepares one first.
   void HandleConnectionAlloc(NSInvocation* invocation);
 
+  // What protocol do we expect to expose over the XPC interface?
+  Protocol* service_protocol_;
+
   // All connection mocks we have currently created.
   std::vector<std::unique_ptr<ConnectionMockRecord>> mocked_connections_;
   size_t next_connection_to_vend_ = 0;
 
   base::scoped_nsobject<id> nsxpcconnection_class_mock_;
-};  // class ScopedXPCUpdateServiceMock
+};  // class ScopedXPCServiceMock
 
 #pragma mark - Helper classes
 

chrome/updater/mac/scoped_xpc_service_mock.mm

@@ -6,7 +6,8 @@
 
 namespace updater {
 
-ScopedXPCUpdateServiceMock::ScopedXPCUpdateServiceMock() {
+ScopedXPCServiceMock::ScopedXPCServiceMock(Protocol* service_protocol)
+    : service_protocol_(service_protocol) {
   @autoreleasepool {
     // Each NSXPCConnection mock must re-mock alloc (when a new mock is created
     // for type T, it un-mocks all previous classmethod mocks on type T - this
@@ -24,10 +25,9 @@ ScopedXPCUpdateServiceMock::ScopedXPCUpdateServiceMock() {
   }
 }
 
-ScopedXPCUpdateServiceMock::~ScopedXPCUpdateServiceMock() = default;
+ScopedXPCServiceMock::~ScopedXPCServiceMock() = default;
 
-void ScopedXPCUpdateServiceMock::HandleConnectionAlloc(
-    NSInvocation* invocation) {
+void ScopedXPCServiceMock::HandleConnectionAlloc(NSInvocation* invocation) {
   ASSERT_TRUE(invocation);
 
   size_t conn_idx = next_connection_to_vend_++;
@@ -43,17 +43,18 @@ void ScopedXPCUpdateServiceMock::HandleConnectionAlloc(
   [invocation setReturnValue:&mock_connection_ptr];
 }
 
-ScopedXPCUpdateServiceMock::RemoteObjectMockRecord::RemoteObjectMockRecord(
-    base::scoped_nsprotocol<id<CRUUpdateChecking>> mock_ptr)
+ScopedXPCServiceMock::RemoteObjectMockRecord::RemoteObjectMockRecord(
+    base::scoped_nsprotocol<id> mock_ptr)
     : mock_object(mock_ptr) {}
 
-ScopedXPCUpdateServiceMock::RemoteObjectMockRecord::~RemoteObjectMockRecord() =
+ScopedXPCServiceMock::RemoteObjectMockRecord::~RemoteObjectMockRecord() =
     default;
 
-ScopedXPCUpdateServiceMock::ConnectionMockRecord::ConnectionMockRecord(
-    ScopedXPCUpdateServiceMock* mock_driver,
+ScopedXPCServiceMock::ConnectionMockRecord::ConnectionMockRecord(
+    ScopedXPCServiceMock* mock_driver,
     size_t index)
-    : index_(index),
+    : service_protocol_(mock_driver->service_protocol_),
+      index_(index),
       mock_connection_(OCMClassMock([NSXPCConnection class]),
                        base::scoped_policy::RETAIN) {
   @autoreleasepool {
@@ -77,7 +78,8 @@ ScopedXPCUpdateServiceMock::ConnectionMockRecord::ConnectionMockRecord(
         .andReturn(mock_connection);
 
     // Expect this connection to receive a correct declaration of the remote
-    // interface: an NSXPCInterface configured with CRUUpdateChecking.
+    // interface: an NSXPCInterface configured with the protocol provided
+    // when |this| was constructed.
     id verifyRemoteInterface = [OCMArg checkWithBlock:^BOOL(id interfaceArg) {
       if (!interfaceArg)
         return NO;
@@ -87,7 +89,7 @@ ScopedXPCUpdateServiceMock::ConnectionMockRecord::ConnectionMockRecord(
       // This test does not verify that the interface is declared correctly
       // around methods that require proxying or collection whitelisting.
       // Use end-to-end tests to detect this.
-      return [interface.protocol isEqual:@protocol(CRUUpdateChecking)];
+      return [interface.protocol isEqual:service_protocol_];
     }];
     OCMExpect([mock_connection setRemoteObjectInterface:verifyRemoteInterface]);
 
@@ -142,46 +144,45 @@ ScopedXPCUpdateServiceMock::ConnectionMockRecord::ConnectionMockRecord(
     // will fail because the object hasn't cleaned up its connections yet.
     OCMExpect([mock_connection invalidate]);
   }  // autoreleasepool
-}  // ScopedXPCUpdateServiceMock::ConnectionMockRecord constructor
+}  // ScopedXPCServiceMock::ConnectionMockRecord constructor
 
-ScopedXPCUpdateServiceMock::ConnectionMockRecord::~ConnectionMockRecord() =
-    default;
+ScopedXPCServiceMock::ConnectionMockRecord::~ConnectionMockRecord() = default;
 
-id ScopedXPCUpdateServiceMock::ConnectionMockRecord::Get() {
+id ScopedXPCServiceMock::ConnectionMockRecord::Get() {
   return mock_connection_.get();
 }
 
-ScopedXPCUpdateServiceMock::ConnectionMockRecord*
-ScopedXPCUpdateServiceMock::PrepareNewMockConnection() {
+ScopedXPCServiceMock::ConnectionMockRecord*
+ScopedXPCServiceMock::PrepareNewMockConnection() {
   mocked_connections_.push_back(
       std::make_unique<ConnectionMockRecord>(this, mocked_connections_.size()));
   return mocked_connections_.back().get();
 }
 
-void ScopedXPCUpdateServiceMock::VerifyAll() {
+void ScopedXPCServiceMock::VerifyAll() {
   for (const auto& connection_ptr : mocked_connections_) {
     connection_ptr->Verify();
   }
 }
 
-size_t ScopedXPCUpdateServiceMock::PreparedConnectionsCount() const {
+size_t ScopedXPCServiceMock::PreparedConnectionsCount() const {
   return mocked_connections_.size();
 }
 
-size_t ScopedXPCUpdateServiceMock::VendedConnectionsCount() const {
+size_t ScopedXPCServiceMock::VendedConnectionsCount() const {
   return next_connection_to_vend_;
 }
 
-ScopedXPCUpdateServiceMock::ConnectionMockRecord*
-ScopedXPCUpdateServiceMock::GetConnection(size_t idx) {
+ScopedXPCServiceMock::ConnectionMockRecord* ScopedXPCServiceMock::GetConnection(
+    size_t idx) {
   if (idx >= mocked_connections_.size()) {
     return nullptr;
   }
   return mocked_connections_[idx].get();
 }
 
-const ScopedXPCUpdateServiceMock::ConnectionMockRecord*
-ScopedXPCUpdateServiceMock::GetConnection(size_t idx) const {
+const ScopedXPCServiceMock::ConnectionMockRecord*
+ScopedXPCServiceMock::GetConnection(size_t idx) const {
   if (idx >= mocked_connections_.size()) {
     return nullptr;
   }
@@ -189,32 +190,30 @@ ScopedXPCUpdateServiceMock::GetConnection(size_t idx) const {
       mocked_connections_[idx].get());
 }
 
-size_t ScopedXPCUpdateServiceMock::ConnectionMockRecord::PreparedObjectsCount()
+size_t ScopedXPCServiceMock::ConnectionMockRecord::PreparedObjectsCount()
     const {
   return remote_object_mocks_.size();
 }
 
-size_t ScopedXPCUpdateServiceMock::ConnectionMockRecord::VendedObjectsCount()
-    const {
+size_t ScopedXPCServiceMock::ConnectionMockRecord::VendedObjectsCount() const {
   return next_mock_to_vend_;
 }
 
-size_t ScopedXPCUpdateServiceMock::ConnectionMockRecord::Index() const {
+size_t ScopedXPCServiceMock::ConnectionMockRecord::Index() const {
   return index_;
 }
 
-ScopedXPCUpdateServiceMock::RemoteObjectMockRecord*
-ScopedXPCUpdateServiceMock::ConnectionMockRecord::PrepareNewMockRemoteObject() {
-  base::scoped_nsprotocol<id<CRUUpdateChecking>> mock(
-      OCMProtocolMock(@protocol(CRUUpdateChecking)),
-      base::scoped_policy::RETAIN);
+ScopedXPCServiceMock::RemoteObjectMockRecord*
+ScopedXPCServiceMock::ConnectionMockRecord::PrepareNewMockRemoteObject() {
+  base::scoped_nsprotocol<id> mock(OCMProtocolMock(service_protocol_),
+                                   base::scoped_policy::RETAIN);
   remote_object_mocks_.push_back(
       std::make_unique<RemoteObjectMockRecord>(mock));
   return remote_object_mocks_.back().get();
 }
 
-ScopedXPCUpdateServiceMock::RemoteObjectMockRecord*
-ScopedXPCUpdateServiceMock::ConnectionMockRecord::GetRemoteObject(
+ScopedXPCServiceMock::RemoteObjectMockRecord*
+ScopedXPCServiceMock::ConnectionMockRecord::GetRemoteObject(
     size_t object_index) {
   if (object_index >= remote_object_mocks_.size()) {
     return nullptr;
@@ -222,8 +221,8 @@ ScopedXPCUpdateServiceMock::ConnectionMockRecord::GetRemoteObject(
   return remote_object_mocks_[object_index].get();
 }
 
-const ScopedXPCUpdateServiceMock::RemoteObjectMockRecord*
-ScopedXPCUpdateServiceMock::ConnectionMockRecord::GetRemoteObject(
+const ScopedXPCServiceMock::RemoteObjectMockRecord*
+ScopedXPCServiceMock::ConnectionMockRecord::GetRemoteObject(
     size_t object_index) const {
   if (object_index >= remote_object_mocks_.size()) {
     return nullptr;
@@ -232,7 +231,7 @@ ScopedXPCUpdateServiceMock::ConnectionMockRecord::GetRemoteObject(
       remote_object_mocks_[object_index].get());
 }
 
-void ScopedXPCUpdateServiceMock::ConnectionMockRecord::Verify() const {
+void ScopedXPCServiceMock::ConnectionMockRecord::Verify() const {
   EXPECT_OCMOCK_VERIFY(mock_connection_.get())
       << "Verification failure in connection " << index_;
   for (size_t idx = 0; idx < remote_object_mocks_.size(); ++idx) {

chrome/updater/mac/update_service_out_of_process_test.mm

@@ -39,7 +39,7 @@ class MacUpdateServiceOutOfProcessTest : public ::testing::Test {
   void InitializeService();
 
   base::test::SingleThreadTaskEnvironment task_environment_;
-  ScopedXPCUpdateServiceMock mock_driver_;
+  ScopedXPCServiceMock mock_driver_ { @protocol (CRUUpdateChecking) };
   std::unique_ptr<base::RunLoop> run_loop_;
   scoped_refptr<UpdateServiceOutOfProcess> service_;
 };  // class MacUpdateOutOfProcessTest
@@ -54,9 +54,9 @@ void MacUpdateServiceOutOfProcessTest::SetUp() {
 }
 
 TEST_F(MacUpdateServiceOutOfProcessTest, NoProductsUpdateAll) {
-  ScopedXPCUpdateServiceMock::ConnectionMockRecord* conn_rec =
+  ScopedXPCServiceMock::ConnectionMockRecord* conn_rec =
       mock_driver_.PrepareNewMockConnection();
-  ScopedXPCUpdateServiceMock::RemoteObjectMockRecord* mock_rec =
+  ScopedXPCServiceMock::RemoteObjectMockRecord* mock_rec =
       conn_rec->PrepareNewMockRemoteObject();
   id<CRUUpdateChecking> mock_remote_object = mock_rec->mock_object.get();