Adds Cleaner tests. Fixes Cleaner, Watcher and Responder

There is no need to return the dictionaries modified by the client since they are "passed-by-reference" in Python.

pisa/cleaner.py

@@ -1,14 +1,13 @@
-import pisa.conf as conf
 from pisa import logging
 
+# Dictionaries in Python are "passed-by-reference", so no return is needed for the Cleaner"
+# https://docs.python.org/3/faq/programming.html#how-do-i-write-a-function-with-output-parameters-call-by-reference
+
 
 class Cleaner:
     @staticmethod
-    def delete_expired_appointment(block, appointments, locator_uuid_map):
+    def delete_expired_appointment(expired_appointments, appointments, locator_uuid_map):
-        to_delete = [uuid for uuid, appointment in appointments.items()
+        for uuid in expired_appointments:
-                     if block["height"] > appointment.end_time + conf.EXPIRY_DELTA]
-
-        for uuid in to_delete:
             locator = appointments[uuid].locator
 
             appointments.pop(uuid)
@@ -22,13 +21,11 @@ class Cleaner:
             logging.info("[Cleaner] end time reached with no match! Deleting appointment {} (uuid: {})".format(locator,
                                                                                                                uuid))
 
-        return appointments, locator_uuid_map
-
     @staticmethod
     def delete_completed_jobs(jobs, tx_job_map, completed_jobs, height):
-        for uuid in completed_jobs:
+        for uuid, confirmations in completed_jobs:
             logging.info("[Cleaner] job completed (uuid = {}). Appointment ended at block {} after {} confirmations"
-                         .format(uuid, jobs[uuid].justice_txid, height, jobs[uuid].confirmations))
+                         .format(uuid, height, confirmations))
 
             # ToDo: #9-add-data-persistence
             justice_txid = jobs[uuid].justice_txid
@@ -41,5 +38,3 @@ class Cleaner:
 
             else:
                 tx_job_map[justice_txid].remove(uuid)
-
-        return jobs, tx_job_map

pisa/responder.py

@@ -120,8 +120,7 @@ class Responder:
                         txs, self.unconfirmed_txs, self.tx_job_map, self.missed_confirmations)
 
                     txs_to_rebroadcast = self.get_txs_to_rebroadcast(txs)
-                    self.jobs, self.tx_job_map = Cleaner.delete_completed_jobs(self.jobs, self.tx_job_map,
+                    Cleaner.delete_completed_jobs(self.jobs, self.tx_job_map, self.get_completed_jobs(height), height)
-                                                                               self.get_completed_jobs(height), height)
 
                     self.rebroadcast(txs_to_rebroadcast)
 
@@ -157,9 +156,10 @@ class Responder:
                 tx = Carrier.get_transaction(job.dispute_txid)
 
                 # FIXME: Should be improved with the librarian
-                if tx is not None and tx.get('confirmations') > MIN_CONFIRMATIONS:
+                confirmations = tx.get('confirmations')
+                if tx is not None and confirmations > MIN_CONFIRMATIONS:
                     # The end of the appointment has been reached
-                    completed_jobs.append(uuid)
+                    completed_jobs.append((uuid, confirmations))
 
         return completed_jobs
 

pisa/watcher.py

@@ -3,10 +3,11 @@ from queue import Queue
 from threading import Thread
 
 from pisa import logging
+from pisa.cleaner import Cleaner
+from pisa.conf import EXPIRY_DELTA
 from pisa.responder import Responder
 from pisa.conf import MAX_APPOINTMENTS
 from pisa.block_processor import BlockProcessor
-from pisa.cleaner import Cleaner
 from pisa.utils.zmq_subscriber import ZMQHandler
 
 
@@ -78,15 +79,17 @@ class Watcher:
             block_hash = self.block_queue.get()
             logging.info("[Watcher] new block received {}".format(block_hash))
 
-            block = BlockProcessor.getblock(block_hash)
+            block = BlockProcessor.get_block(block_hash)
 
             if block is not None:
                 txids = block.get('tx')
 
                 logging.info("[Watcher] list of transactions: {}".format(txids))
 
-                self.appointments, self.locator_uuid_map = Cleaner.delete_expired_appointment(
+                expired_appointments = [uuid for uuid, appointment in self.appointments.items()
-                    block, self.appointments, self.locator_uuid_map)
+                                        if block["height"] > appointment.end_time + EXPIRY_DELTA]
+
+                Cleaner.delete_expired_appointment(expired_appointments, self.appointments, self.locator_uuid_map)
 
                 potential_matches = BlockProcessor.get_potential_matches(txids, self.locator_uuid_map)
                 matches = BlockProcessor.get_matches(potential_matches, self.locator_uuid_map, self.appointments)

tests/test_cleaner.py

@@ -0,0 +1,87 @@
+import random
+from os import urandom
+from uuid import uuid4
+from binascii import hexlify
+
+from pisa import logging
+from pisa.responder import Job
+from pisa.cleaner import Cleaner
+from pisa.appointment import Appointment
+
+CONFIRMATIONS = 6
+ITEMS = 10
+MAX_ITEMS = 100
+ITERATIONS = 1000
+
+
+def set_up_appointments(total_appointments):
+    appointments = dict()
+    locator_uuid_map = dict()
+
+    for _ in range(total_appointments):
+        uuid = uuid4().hex
+        locator = hexlify(urandom(64))
+
+        appointments[uuid] = Appointment(locator, None, None, None, None, None, None)
+        locator_uuid_map[locator] = [uuid]
+
+        # Each locator can have more than one uuid assigned to it. Do a coin toss to add multiple ones
+        while random.randint(0, 1):
+            uuid = uuid4().hex
+
+            appointments[uuid] = Appointment(locator, None, None, None, None, None, None)
+            locator_uuid_map[locator].append(uuid)
+
+    return appointments, locator_uuid_map
+
+
+def set_up_jobs(total_jobs):
+    jobs = dict()
+    tx_job_map = dict()
+
+    for _ in range(total_jobs):
+        uuid = uuid4().hex
+        txid = hexlify(urandom(64))
+
+        # Assign both justice_txid and dispute_txid the same id (it shouldn't matter)
+        jobs[uuid] = Job(txid, txid, None, None, None)
+        tx_job_map[txid] = [uuid]
+
+        # Each justice_txid can have more than one uuid assigned to it. Do a coin toss to add multiple ones
+        while random.randint(0, 1):
+            uuid = uuid4().hex
+
+            jobs[uuid] = Job(txid, txid, None, None, None)
+            tx_job_map[txid].append(uuid)
+
+    return jobs, tx_job_map
+
+
+def test_delete_expired_appointment():
+    appointments, locator_uuid_map = set_up_appointments(MAX_ITEMS)
+    expired_appointments = random.sample(list(appointments.keys()), k=ITEMS)
+
+    Cleaner.delete_expired_appointment(expired_appointments, appointments, locator_uuid_map)
+
+    assert not set(expired_appointments).issubset(appointments.keys())
+
+
+def test_delete_completed_jobs():
+    jobs, tx_job_map = set_up_jobs(MAX_ITEMS)
+    selected_jobs = random.sample(list(jobs.keys()), k=ITEMS)
+
+    completed_jobs = [(job, 6) for job in selected_jobs]
+
+    Cleaner.delete_completed_jobs(jobs, tx_job_map, completed_jobs, 0)
+
+    assert not set(completed_jobs).issubset(jobs.keys())
+
+
+logging.getLogger().disabled = True
+
+for _ in range(ITERATIONS):
+    test_delete_expired_appointment()
+
+for _ in range(ITERATIONS):
+    test_delete_completed_jobs()
+