Code review comment for lp:~therve/landscape-client/fake-package-reporter

Looks good! I have a few comments though.

[1]

+class FakePackageStore(PackageStore):

It'd be good to add direct tests for this class.

[2]

+ if not os.path.exists(global_store):
+ return succeed(None)

Why is this needed?

[3]

+ global_db = sqlite3.connect(global_store)
+ cursor = global_db.cursor()
+ all_message_ids = set(
+ row[0] for row in
+ cursor.execute("SELECT id FROM message").fetchall())

Can't FakePackageStore.get_message_ids be used instead?

[4]

+ messages = list(
+ (row[0], row[1]) for row in
+ cursor.execute(
+ "SELECT id, data FROM message WHERE id IN (%s) "
+ "ORDER BY id" % params, tuple(not_sent)).fetchall())

It'd be nice to encapsulate this if FakePackageStore, and test it separately.

[5]

+ lambda x, message=message:
+ self._broker.send_message(message, True))

Ah, nice trick to get the message variable in the closure.

[6]

+ def send_message(self, message):
+ return succeed(None)
+
+ def use_hash_id_db(self):
+ return succeed(None)
+
+ def request_unknown_hashes(self):
+ return succeed(None)
+
+ def remove_expired_hash_id_requests(self):
+ return succeed(None)
+
+ def handle_task(self, task):
+ return succeed(None)
+

Instead of this, why not override the run() method directly? It would avoid having to modify the fake reporter in case we add methods to the real one.