Merge pull request #918 from tahoe-lafs/3521.test_storage_client-no-mock

Remove Mock usage from test_storage_client

Fixes: ticket:3521

src/allmydata/interfaces.py

@@ -521,7 +521,6 @@ class IStorageBroker(Interface):
           oldest_supported: the peer's oldest supported version, same
 
           rref: the RemoteReference, if connected, otherwise None
-          remote_host: the IAddress, if connected, otherwise None
 
         This method is intended for monitoring interfaces, such as a web page
         that describes connecting and connected peers.

src/allmydata/storage_client.py

@@ -154,6 +154,9 @@ class StorageFarmBroker(service.MultiService):
     I'm also responsible for subscribing to the IntroducerClient to find out
     about new servers as they are announced by the Introducer.
 
+    :ivar _tub_maker: A one-argument callable which accepts a dictionary of
+        "handler overrides" and returns a ``foolscap.api.Tub``.
+
     :ivar StorageClientConfig storage_client_config: Values from the node
         configuration file relating to storage behavior.
     """
@@ -690,7 +693,6 @@ class NativeStorageServer(service.MultiService):
     @ivar nickname: the server's self-reported nickname (unicode), same
 
     @ivar rref: the RemoteReference, if connected, otherwise None
-    @ivar remote_host: the IAddress, if connected, otherwise None
     """
 
     VERSION_DEFAULTS = UnicodeKeyDict({
@@ -716,7 +718,6 @@ class NativeStorageServer(service.MultiService):
 
         self.last_connect_time = None
         self.last_loss_time = None
-        self.remote_host = None
         self._rref = None
         self._is_connected = False
         self._reconnector = None
@@ -825,8 +826,6 @@ class NativeStorageServer(service.MultiService):
         return None
     def get_announcement(self):
         return self.announcement
-    def get_remote_host(self):
-        return self.remote_host
 
     def get_connection_status(self):
         last_received = None
@@ -874,7 +873,6 @@ class NativeStorageServer(service.MultiService):
                 level=log.NOISY, parent=lp)
 
         self.last_connect_time = time.time()
-        self.remote_host = rref.getLocationHints()
         self._rref = rref
         self._is_connected = True
         rref.notifyOnDisconnect(self._lost)
@@ -900,7 +898,6 @@ class NativeStorageServer(service.MultiService):
         # get_connected_servers() or get_servers_for_psi()) can continue to
         # use s.get_rref().callRemote() and not worry about it being None.
         self._is_connected = False
-        self.remote_host = None
 
     def stop_connecting(self):
         # used when this descriptor has been superceded by another

src/allmydata/test/test_introducer.py

@@ -744,8 +744,6 @@ class SystemTest(SystemTestMixin, AsyncTestCase):
 class FakeRemoteReference(object):
     def notifyOnDisconnect(self, *args, **kwargs): pass
     def getRemoteTubID(self): return "62ubehyunnyhzs7r6vdonnm2hpi52w6y"
-    def getLocationHints(self): return ["tcp:here.example.com:1234",
-                                        "tcp:there.example.com2345"]
     def getPeer(self): return address.IPv4Address("TCP", "remote.example.com",
                                                   3456)
 

src/allmydata/test/test_storage_client.py

@@ -17,7 +17,6 @@ from json import (
 )
 
 import hashlib
-from mock import Mock
 from fixtures import (
     TempDir,
 )
@@ -44,12 +43,20 @@ from hyperlink import (
     URL,
 )
 
+import attr
+
+from twisted.internet.interfaces import (
+    IStreamClientEndpoint,
+)
 from twisted.application.service import (
     Service,
 )
 
 from twisted.trial import unittest
-from twisted.internet.defer import succeed, inlineCallbacks
+from twisted.internet.defer import (
+    Deferred,
+    inlineCallbacks,
+)
 from twisted.python.filepath import (
     FilePath,
 )
@@ -57,7 +64,11 @@ from twisted.python.filepath import (
 from foolscap.api import (
     Tub,
 )
+from foolscap.ipb import (
+    IConnectionHintHandler,
+)
 
+from .no_network import LocalWrapper
 from .common import (
     EMPTY_CLIENT_CONFIG,
     SyncTestCase,
@@ -65,6 +76,7 @@ from .common import (
     UseTestPlugins,
     UseNode,
     SameProcessStreamEndpointAssigner,
+    MemoryIntroducerClient,
 )
 from .common_web import (
     do_http,
@@ -83,6 +95,9 @@ from allmydata.storage_client import (
     _FoolscapStorage,
     _NullStorage,
 )
+from ..storage.server import (
+    StorageServer,
+)
 from allmydata.interfaces import (
     IConnectionStatus,
     IStorageServer,
@@ -505,14 +520,68 @@ class StoragePluginWebPresence(AsyncTestCase):
         )
 
 
-def make_broker(tub_maker=lambda h: Mock()):
+_aCertPEM = Tub().myCertificate.dumpPEM()
+def new_tub():
+    """
+    Make a new ``Tub`` with a hard-coded private key.
+    """
+    # Use a private key / certificate generated by Tub how it wants.  But just
+    # re-use the same one every time so we don't waste a lot of time
+    # generating them over and over in the tests.
+    return Tub(certData=_aCertPEM)
+
+
+def make_broker(tub_maker=None):
     """
     Create a ``StorageFarmBroker`` with the given tub maker and an empty
     client configuration.
     """
+    if tub_maker is None:
+        tub_maker = lambda handler_overrides: new_tub()
     return StorageFarmBroker(True, tub_maker, EMPTY_CLIENT_CONFIG)
 
 
+@implementer(IStreamClientEndpoint)
+@attr.s
+class SpyEndpoint(object):
+    """
+    Observe and record connection attempts.
+
+    :ivar list _append: A callable that accepts two-tuples.  For each
+        attempted connection, it will be called with ``Deferred`` that was
+        returned and the ``Factory`` that was passed in.
+    """
+    _append = attr.ib()
+
+    def connect(self, factory):
+        """
+        Record the connection attempt.
+
+        :return: A ``Deferred`` that ``SpyEndpoint`` will not fire.
+        """
+        d = Deferred()
+        self._append((d, factory))
+        return d
+
+
+@implementer(IConnectionHintHandler)
+@attr.s
+class SpyHandler(object):
+    """
+    A Foolscap connection hint handler for the "spy" hint type.  Connections
+    are handled by just observing and recording them.
+
+    :ivar list _connects: A list containing one element for each connection
+        attempted with this handler.  Each element is a two-tuple of the
+        ``Deferred`` that was returned from ``connect`` and the factory that
+        was passed to ``connect``.
+    """
+    _connects = attr.ib(default=attr.Factory(list))
+
+    def hint_to_endpoint(self, hint, reactor, update_status):
+        return (SpyEndpoint(self._connects.append), hint)
+
+
 class TestStorageFarmBroker(unittest.TestCase):
 
     def test_static_servers(self):
@@ -585,18 +654,38 @@ storage:
 
     @inlineCallbacks
     def test_threshold_reached(self):
-        introducer = Mock()
+        """
+        ``StorageFarmBroker.when_connected_enough`` returns a ``Deferred`` which
+        only fires after the ``StorageFarmBroker`` has established at least as
+        many connections as requested.
+        """
+        introducer = MemoryIntroducerClient(
+            new_tub(),
+            SOME_FURL,
+            b"",
+            None,
+            None,
+            None,
+            None,
+        )
         new_tubs = []
         def make_tub(*args, **kwargs):
             return new_tubs.pop()
         broker = make_broker(make_tub)
+        # Start the broker so that it will start Tubs attached to it so they
+        # will attempt to make connections as necessary so that we can observe
+        # those connections.
+        broker.startService()
+        self.addCleanup(broker.stopService)
         done = broker.when_connected_enough(5)
         broker.use_introducer(introducer)
         # subscribes to "storage" to learn of new storage nodes
-        subscribe = introducer.mock_calls[0]
-        self.assertEqual(subscribe[0], 'subscribe_to')
-        self.assertEqual(subscribe[1][0], 'storage')
-        got_announcement = subscribe[1][1]
+        [subscribe] = introducer.subscribed_to
+        self.assertEqual(
+            subscribe.service_name,
+            "storage",
+        )
+        got_announcement = subscribe.cb
 
         data = {
             "service-name": "storage",
@@ -605,15 +694,25 @@ storage:
         }
 
         def add_one_server(x):
-            data["anonymous-storage-FURL"] = b"pb://%s@nowhere/fake" % (base32.b2a(b"%d" % x),)
-            tub = Mock()
+            data["anonymous-storage-FURL"] = b"pb://%s@spy:nowhere/fake" % (base32.b2a(b"%d" % x),)
+            tub = new_tub()
+            connects = []
+            spy = SpyHandler(connects)
+            tub.addConnectionHintHandler("spy", spy)
             new_tubs.append(tub)
             got_announcement(b'v0-1234-%d' % x, data)
-            self.assertEqual(tub.mock_calls[-1][0], 'connectTo')
-            got_connection = tub.mock_calls[-1][1][1]
-            rref = Mock()
-            rref.callRemote = Mock(return_value=succeed(1234))
-            got_connection(rref)
+
+            self.assertEqual(
+                1, len(connects),
+                "Expected one connection attempt, got {!r} instead".format(connects),
+            )
+
+            # Skip over all the Foolscap negotiation.  It's complex with lots
+            # of pieces and I don't want to figure out how to fake
+            # it. -exarkun
+            native = broker.servers[b"v0-1234-%d" % (x,)]
+            rref = LocalWrapper(StorageServer(self.mktemp(), b"x" * 20))
+            native._got_connection(rref)
 
         # first 4 shouldn't trigger connected_threashold
         for x in range(4):