Skip to content

Commit

Permalink
Refactor rename methods to fix mistaken method override
Browse files Browse the repository at this point in the history
  • Loading branch information
gonzalo-bulnes committed Jan 16, 2023
1 parent d05e446 commit 9ff352c
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 54 deletions.
16 changes: 8 additions & 8 deletions securedrop_client/export/disk.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ class Disk(QObject):
export_done = pyqtSignal()
export_failed = pyqtSignal()

client_connected = pyqtSignal()
last_client_disconnected = pyqtSignal()
client_started_watching = pyqtSignal()
last_client_stopped_watching = pyqtSignal()

Status = NewType("Status", str)
StatusUnknown = Status("unknown-isw32")
Expand Down Expand Up @@ -57,8 +57,8 @@ def __init__(
self._poller.wait_on(self._export_service.luks_encrypted_disk_found)
self._poller.wait_on(self._export_service.luks_encrypted_disk_not_found)

self._poller.start_on(self.client_connected)
self._poller.pause_on(self.last_client_disconnected)
self._poller.start_on(self.client_started_watching)
self._poller.pause_on(self.last_client_stopped_watching)

self._export_service.luks_encrypted_disk_not_found.connect(
lambda error: self._on_luks_encrypted_disk_not_found(error)
Expand All @@ -75,15 +75,15 @@ def last_error(self) -> Optional[CLIError]:
return self._last_error # FIXME Returning the CLIError type is an abstraction leak.

@pyqtSlot()
def connect(self) -> None:
def watch(self) -> None:
self._connected_clients += 1
self.client_connected.emit()
self.client_started_watching.emit()

@pyqtSlot()
def disconnect(self) -> None:
def stop_watching(self) -> None:
self._connected_clients -= 1
if self._connected_clients < 1:
self.last_client_disconnected.emit()
self.last_client_stopped_watching.emit()

def export_on(self, signal: pyqtBoundSignal) -> None:
"""Allow to export files, in a thread-safe manner."""
Expand Down
16 changes: 8 additions & 8 deletions securedrop_client/export/printer.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ class Printer(QObject):
job_done = pyqtSignal()
job_failed = pyqtSignal()

client_connected = pyqtSignal()
last_client_disconnected = pyqtSignal()
client_started_watching = pyqtSignal()
last_client_stopped_watching = pyqtSignal()

Status = NewType("Status", str)
StatusUnknown = Status("unknown-sf5fd")
Expand Down Expand Up @@ -61,8 +61,8 @@ def __init__(
self._poller.wait_on(self._printing_service.printer_not_found_ready)
self._poller.wait_on(self._printing_service.printer_found_ready)

self._poller.start_on(self.client_connected)
self._poller.pause_on(self.last_client_disconnected)
self._poller.start_on(self.client_started_watching)
self._poller.pause_on(self.last_client_stopped_watching)

# The printing service is not up-to-date on the printing queue terminology.
self._printing_service.printer_not_found_ready.connect(
Expand All @@ -80,15 +80,15 @@ def last_error(self) -> Optional[CLIError]:
return self._last_error # FIXME Returning the CLIError type is an abstraction leak.

@pyqtSlot()
def connect(self) -> None:
def watch(self) -> None:
self._connected_clients += 1
self.client_connected.emit()
self.client_started_watching.emit()

@pyqtSlot()
def disconnect(self) -> None:
def stop_watching(self) -> None:
self._connected_clients -= 1
if self._connected_clients < 1:
self.last_client_disconnected.emit()
self.last_client_stopped_watching.emit()

def enqueue_job_on(self, signal: pyqtBoundSignal) -> None:
"""Allow to enqueue printing jobs, in a thread-safe manner."""
Expand Down
39 changes: 20 additions & 19 deletions tests/export/test_disk.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ def test_disk_status_is_unknown_by_default(self):

self.assertEqual(Disk.StatusUnknown, disk.status)

def test_disk_status_tracks_export_service_responses_when_connected(self):
def test_disk_status_tracks_export_service_responses_when_watched(self):
responses = [
Disk.StatusLUKSEncrypted,
Disk.StatusUnreachable,
Expand All @@ -186,23 +186,23 @@ def test_disk_status_tracks_export_service_responses_when_connected(self):
self.assertEqual(
0,
len(export_service_response),
"Expected export service to receive no queries before the disk is connected, and emit no responses.", # noqa: E501
"Expected export service to receive no queries before the disk is being watched, and emit no responses.", # noqa: E501
)
self.assertEqual(
Disk.StatusUnknown,
disk.status,
"Expected default disk status to be Disk.StatusUnknown, was not.",
)

disk.connect() # Action!
disk.watch() # Action!

# The first query should be issued immediately.
# After that, the dummy export service responds blazing fast.
export_service_response.wait(CHECK_EXECUTION_TIME) # milliseconds
self.assertEqual(
1,
len(export_service_response),
"Expected exactly 1 query to the export service, and 1 response immediately after the disk was connected.", # noqa: E501
"Expected exactly 1 query to the export service, and 1 response immediately after the disk started being watched.", # noqa: E501
)
self.assertEqual(
Disk.StatusLUKSEncrypted,
Expand Down Expand Up @@ -256,7 +256,7 @@ def test_disk_status_tracks_export_service_responses_when_connected(self):
"Expected disk status to track the last response, did not.",
)

def test_disk_status_stops_tracking_export_service_responses_when_disconnected(self):
def test_disk_status_stops_tracking_export_service_responses_when_not_watched(self):
responses = [
Disk.StatusLUKSEncrypted,
Disk.StatusUnreachable,
Expand All @@ -276,48 +276,48 @@ def test_disk_status_stops_tracking_export_service_responses_when_disconnected(s
self.assertEqual(
0,
len(export_service_response),
"Expected export service to receive no queries before the disk is connected, and emit no responses.", # noqa: E501
"Expected export service to receive no queries before the disk is being watched, and emit no responses.", # noqa: E501
)
self.assertEqual(
Disk.StatusUnknown,
disk.status,
"Expected default disk status to be Disk.StatusUnknown, was not.",
)

disk.connect() # Action!
disk.watch() # Action!

# The first query should be issued immediately.
# After that, the dummy export service responds blazing fast.
export_service_response.wait(CHECK_EXECUTION_TIME) # milliseconds
self.assertEqual(
1,
len(export_service_response),
"Expected exactly 1 query to the export service, and 1 response immediately after the disk was connected.", # noqa: E501
"Expected exactly 1 query to the export service, and 1 response immediately after the disk started being watched.", # noqa: E501
)
self.assertEqual(
Disk.StatusLUKSEncrypted,
disk.status,
"Expected disk status to track the last response, did not.",
)

disk.disconnect()
disk.stop_watching()

self.assertEqual(
Disk.StatusUnknown,
disk.status,
"Expected disk status to become unknown as soon as disconnected.",
"Expected disk status to become unknown as soon as stopped being watched.",
)

export_service_response.wait(POLLING_INTERVAL + CHECK_EXECUTION_TIME) # will time out
self.assertEqual(
1,
len(export_service_response),
"Expected no new query to the export service after disconnection (total 1 query and 1 response)", # noqa: E501
"Expected no new query to the export service after the disk stopped being watched (total 1 query and 1 response)", # noqa: E501
)
self.assertEqual(
Disk.StatusUnknown,
disk.status,
"Expected disk status to remain unknown until reconnected.",
"Expected disk status to remain unknown until being watched again.",
)

def test_disk_signals_changes_in_disk_status(self):
Expand Down Expand Up @@ -381,19 +381,20 @@ def test_disk_signals_changes_in_disk_status(self):
)

# This last segment is admittedly awkward. We want to make sure that
# the signal is emitted when pausing the disk. Even though the disk connection
# is not the point of this test, we do have to connect to be able to disconnect it.
# the signal is emitted when pausing the disk. Even though the disk watching
# is not the point of this test, we do have to watch in order to be able
# to stop watching it.
#
# The connection triggers a query to the disk_service. To ensure that
# connecting the disk doesn't have any visible side-effects, the dummy service
# Watching triggers a query to the disk_service. To ensure that
# watching the disk doesn't have any visible side-effects, the dummy service
# is configured to respond that the disk wasn't found LUKS-encrypted.
disk.connect()
disk.disconnect()
disk.watch()
disk.stop_watching()
disk_status_changed_emissions.wait(POLLING_INTERVAL)
self.assertEqual(
Disk.StatusUnknown,
disk.status,
"Expected disk status to become Disk.StatusUnknown as soon as disconnected, was not.", # noqa: E501
"Expected disk status to become Disk.StatusUnknown as soon as not being watched, was not.", # noqa: E501
)
self.assertEqual(
4,
Expand Down
39 changes: 20 additions & 19 deletions tests/export/test_printer.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ def test_printer_status_is_unknown_by_default(self):

self.assertEqual(Printer.StatusUnknown, printer.status)

def test_printer_status_tracks_printing_service_responses_when_connected(self):
def test_printer_status_tracks_printing_service_responses_when_watched(self):
responses = [
Printer.StatusReady,
Printer.StatusUnreachable,
Expand Down Expand Up @@ -184,18 +184,18 @@ def test_printer_status_tracks_printing_service_responses_when_connected(self):
self.assertEqual(
0,
len(printing_service_response),
"Expected printing service to receive no queries before the printer is connected, and emit no responses.", # noqa: E501
"Expected printing service to receive no queries before the printer is being watched, and emit no responses.", # noqa: E501
)

printer.connect() # Action!
printer.watch() # Action!

# The first query should be issued immediately.
# After that, the dummy printing service responds blazing fast.
printing_service_response.wait(CHECK_EXECUTION_TIME) # milliseconds
self.assertEqual(
1,
len(printing_service_response),
"Expected exactly 1 query to the printing service, and 1 response immediately after the printer was connected.", # noqa: E501
"Expected exactly 1 query to the printing service, and 1 response immediately after the printer started being watched.", # noqa: E501
)
self.assertEqual(
Printer.StatusReady,
Expand Down Expand Up @@ -249,7 +249,7 @@ def test_printer_status_tracks_printing_service_responses_when_connected(self):
"Expected printer status to track the last response, did not.",
)

def test_printer_status_stops_tracking_printing_service_responses_when_disconnected(self):
def test_printer_status_stops_tracking_printing_service_responses_when_not_watched(self):
responses = [
Printer.StatusReady,
Printer.StatusUnreachable,
Expand All @@ -275,43 +275,43 @@ def test_printer_status_stops_tracking_printing_service_responses_when_disconnec
self.assertEqual(
0,
len(printing_service_response),
"Expected printing service to receive no queries before the printer is connected, and emit no responses.", # noqa: E501
"Expected printing service to receive no queries before the printer is watched, and emit no responses.", # noqa: E501
)

printer.connect() # Action!
printer.watch() # Action!

# The first query should be issued immediately.
# After that, the dummy printing service responds blazing fast.
printing_service_response.wait(CHECK_EXECUTION_TIME) # milliseconds
self.assertEqual(
1,
len(printing_service_response),
"Expected exactly 1 query to the printing service, and 1 response immediately after the printer was connected.", # noqa: E501
"Expected exactly 1 query to the printing service, and 1 response immediately after the printer started being watched.", # noqa: E501
)
self.assertEqual(
Printer.StatusReady,
printer.status,
"Expected printer status to track the last response, did not.",
)

printer.disconnect()
printer.stop_watching()

self.assertEqual(
Printer.StatusUnknown,
printer.status,
"Expected printer status to become unknown as soon as disconnected.",
"Expected printer status to become unknown as soon as watching stopped.",
)

printing_service_response.wait(POLLING_INTERVAL + CHECK_EXECUTION_TIME) # will time out
self.assertEqual(
1,
len(printing_service_response),
"Expected no new query to the printing service after disconnection (total 1 query and 1 response)", # noqa: E501
"Expected no new query to the printing service after watching has stopped (total 1 query and 1 response)", # noqa: E501
)
self.assertEqual(
Printer.StatusUnknown,
printer.status,
"Expected printer status to remain unknown until reconnected.",
"Expected printer status to remain unknown until being watched again.",
)

def test_printer_signals_changes_in_printer_status(self):
Expand Down Expand Up @@ -375,19 +375,20 @@ def test_printer_signals_changes_in_printer_status(self):
)

# This last segment is admittedly awkward. We want to make sure that
# the signal is emitted when pausing the printer. Even though the printer connection
# is not the point of this test, we do have to connect to be able to disconnect it.
# the signal is emitted when pausing the printer. Even though the printer watching
# is not the point of this test, we do have to watch in order to be able
# to stop watching it.
#
# The connection triggers a query to the printer_service. To ensure that
# connecting the printer doesn't have any visible side-effects, the dummy service
# Watching triggers a query to the printer_service. To ensure that
# watching the printer doesn't have any visible side-effects, the dummy service
# is configured to respond that the printer wasn't found ready.
printer.connect()
printer.disconnect()
printer.watch()
printer.stop_watching()
printer_status_changed_emissions.wait(POLLING_INTERVAL)
self.assertEqual(
Printer.StatusUnknown,
printer.status,
"Expected printer status to become Printer.StatusUnknown as soon as disconnected, was not.", # noqa: E501
"Expected printer status to become Printer.StatusUnknown as soon as watching stopped, was not.", # noqa: E501
)
self.assertEqual(
4,
Expand Down

0 comments on commit 9ff352c

Please sign in to comment.