tests: Add full test coverage for updater checks

Fully test the update check logic, by introducing several Qt tests.
Also, improve the `UpdaterThread.get_letest_info()` method, that gets
the latest version and changelog from GitHub, with several checks.
These checks are also tested in our newly added tests.

dangerzone/gui/updater.py

@@ -1,5 +1,6 @@
 """A module that contains the logic for checking for updates."""
 
+import json
 import logging
 import platform
 import sys
@@ -97,6 +98,7 @@ class UpdaterThread(QtCore.QThread):
     GH_RELEASE_URL = (
         "https://api.github.com/repos/freedomofpress/dangerzone/releases/latest"
     )
+    REQ_TIMEOUT = 15
 
     def __init__(self, dangerzone: DangerzoneGui):
         super().__init__()
@@ -201,14 +203,30 @@ class UpdaterThread(QtCore.QThread):
         to the users.
         """
         try:
-            # TODO: Set timeout.
-            info = requests.get(self.GH_RELEASE_URL).json()
+            res = requests.get(self.GH_RELEASE_URL, timeout=self.REQ_TIMEOUT)
         except Exception as e:
-            # TODO:: Handle errors.
-            raise
+            raise RuntimeError(
+                f"Encountered an exception while querying {self.GH_RELEASE_URL}: {e}"
+            )
 
-        version = info["tag_name"].lstrip("v")
-        changelog = markdown.markdown(info["body"])
+        if res.status_code != 200:
+            raise RuntimeError(
+                f"Encountered an HTTP {res.status_code} error while querying"
+                f" {self.GH_RELEASE_URL}"
+            )
+
+        try:
+            info = res.json()
+        except json.JSONDecodeError as e:
+            raise ValueError(f"Received a non-JSON response from {self.GH_RELEASE_URL}")
+
+        try:
+            version = info["tag_name"].lstrip("v")
+            changelog = markdown.markdown(info["body"])
+        except KeyError as e:
+            raise ValueError(
+                f"Missing required fields in JSON response from {self.GH_RELEASE_URL}"
+            )
 
         return UpdateReport(version=version, changelog=changelog)
 

tests/gui/test_main_window.py

@@ -1,26 +1,22 @@
+import time
+import typing
+
+from PySide6 import QtCore, QtWidgets
 from pytest import MonkeyPatch, fixture
 from pytest_mock import MockerFixture
 from pytestqt.qtbot import QtBot
 
 from dangerzone.gui import MainWindow
-from dangerzone.gui import updater as updater_mod
-from dangerzone.gui.main_window import *
+from dangerzone.gui import main_window as main_window_module
+from dangerzone.gui import updater as updater_module
+from dangerzone.gui.logic import DangerzoneGui
+from dangerzone.gui.main_window import ContentWidget
 from dangerzone.gui.updater import UpdateReport, UpdaterThread
 from dangerzone.util import get_version
 
 from .. import sample_doc, sample_pdf
-from . import qt_updater, updater
-from .test_updater import default_updater_settings
-
-# FIXME: See https://github.com/freedomofpress/dangerzone/issues/320 for more details.
-if typing.TYPE_CHECKING:
-    from PySide2 import QtCore
-else:
-    try:
-        from PySide6 import QtCore, QtGui, QtWidgets
-    except ImportError:
-        from PySide2 import QtCore, QtGui, QtWidgets
-
+from . import qt_updater as updater
+from .test_updater import assert_report_equal, default_updater_settings
 
 ##
 ## Widget Fixtures
@@ -28,7 +24,7 @@ else:
 
 
 @fixture
-def content_widget(qtbot: QtBot, mocker: MockerFixture) -> QtWidgets.QWidget:
+def content_widget(qtbot: QtBot, mocker: MockerFixture) -> ContentWidget:
     # Setup
     mock_app = mocker.MagicMock()
     dummy = mocker.MagicMock()
@@ -38,63 +34,280 @@ def content_widget(qtbot: QtBot, mocker: MockerFixture) -> QtWidgets.QWidget:
     return w
 
 
-def test_qt(
+def test_default_menu(
     qtbot: QtBot,
-    qt_updater: UpdaterThread,
+    updater: UpdaterThread,
+) -> None:
+    """Check that the default menu entries are in order."""
+    updater.dangerzone.settings.set("updater_check", True)
+
+    window = MainWindow(updater.dangerzone)
+    menu_actions = window.hamburger_button.menu().actions()
+    assert len(menu_actions) == 3
+
+    toggle_updates_action = menu_actions[0]
+    assert toggle_updates_action.text() == "Check for updates"
+    assert toggle_updates_action.isChecked()
+
+    separator = menu_actions[1]
+    assert separator.isSeparator()
+
+    exit_action = menu_actions[2]
+    assert exit_action.text() == "Exit"
+
+    toggle_updates_action.trigger()
+    assert not toggle_updates_action.isChecked()
+    assert updater.dangerzone.settings.get("updater_check") == False
+
+
+def test_no_update(
+    qtbot: QtBot,
+    updater: UpdaterThread,
     monkeypatch: MonkeyPatch,
     mocker: MockerFixture,
 ) -> None:
-    updater = qt_updater
+    """Test that when no update has been detected, the user is not alerted."""
+    # Check that when no update is detected, e.g., due to update cooldown, an empty
+    # report is received that does not affect the menu entries.
+    curtime = int(time.time())
     updater.dangerzone.settings.set("updater_check", True)
-    updater.dangerzone.settings.set("updater_last_check", 0)
+    updater.dangerzone.settings.set("updater_errors", 9)
+    updater.dangerzone.settings.set("updater_last_check", curtime)
+
     expected_settings = default_updater_settings()
     expected_settings["updater_check"] = True
-
-    mock_upstream_info = {"tag_name": f"v{get_version()}", "body": "changelog"}
-
-    # Always assume that we can perform multiple update checks in a row.
-    monkeypatch.setattr(updater, "_should_postpone_update_check", lambda: False)
-
-    # Make requests.get().json() return the above dictionary.
-    requests_mock = mocker.MagicMock()
-    requests_mock().json.return_value = mock_upstream_info
-    monkeypatch.setattr(updater_mod.requests, "get", requests_mock)
+    expected_settings["updater_errors"] = 0  # errors must be cleared
+    expected_settings["updater_last_check"] = curtime
 
     window = MainWindow(updater.dangerzone)
     window.register_update_handler(updater.finished)
+    handle_updates_spy = mocker.spy(window, "handle_updates")
+
+    menu_actions_before = window.hamburger_button.menu().actions()
+
     with qtbot.waitSignal(updater.finished) as blocker:
         updater.start()
 
-    assert len(window.hamburger_button.menu().actions()) == 3
+    # Check that the callback function gets an empty report.
+    handle_updates_spy.assert_called_once()
+    assert_report_equal(handle_updates_spy.call_args.args[0], UpdateReport())
+
+    # Check that the menu entries remain exactly the same.
+    menu_actions_after = window.hamburger_button.menu().actions()
+    assert menu_actions_before == menu_actions_after
+
+    # Check that any previous update errors are cleared.
+    assert updater.dangerzone.settings.get_updater_settings() == expected_settings
+
+
+def test_update_detected(
+    qtbot: QtBot,
+    updater: UpdaterThread,
+    monkeypatch: MonkeyPatch,
+    mocker: MockerFixture,
+) -> None:
+    """Test that a newly detected version leads to a notification to the user."""
+    updater.dangerzone.settings.set("updater_check", True)
+    updater.dangerzone.settings.set("updater_last_check", 0)
+    updater.dangerzone.settings.set("updater_errors", 9)
+
+    # Make requests.get().json() return the following dictionary.
+    mock_upstream_info = {"tag_name": "99.9.9", "body": "changelog"}
+    mocker.patch("dangerzone.gui.updater.requests.get")
+    requests_mock = updater_module.requests.get
+    requests_mock().status_code = 200  # type: ignore [call-arg]
+    requests_mock().json.return_value = mock_upstream_info  # type: ignore [attr-defined, call-arg]
+
+    window = MainWindow(updater.dangerzone)
+    window.register_update_handler(updater.finished)
+    handle_updates_spy = mocker.spy(window, "handle_updates")
+    load_svg_spy = mocker.spy(window, "load_svg_image")
+
+    menu_actions_before = window.hamburger_button.menu().actions()
+
+    with qtbot.waitSignal(updater.finished) as blocker:
+        updater.start()
+
+    menu_actions_after = window.hamburger_button.menu().actions()
+
+    # Check that the callback function gets an update report.
+    handle_updates_spy.assert_called_once()
+    assert_report_equal(
+        handle_updates_spy.call_args.args[0], UpdateReport("99.9.9", "<p>changelog</p>")
+    )
+
+    # Check that the settings have been updated properly.
+    expected_settings = default_updater_settings()
+    expected_settings["updater_check"] = True
     expected_settings["updater_last_check"] = updater.dangerzone.settings.get(
         "updater_last_check"
     )
-    assert updater.dangerzone.settings.get_updater_settings() == expected_settings
-
-    mock_upstream_info["tag_name"] = "v99.9.9"
-    mock_upstream_info["changelog"] = "changelog"
     expected_settings["updater_latest_version"] = "99.9.9"
     expected_settings["updater_latest_changelog"] = "<p>changelog</p>"
+    expected_settings["updater_errors"] = 0
+    assert updater.dangerzone.settings.get_updater_settings() == expected_settings
+
+    # Check that the hamburger icon has changed with the expected SVG image.
+    assert load_svg_spy.call_count == 2
+    assert load_svg_spy.call_args_list[0].args[0] == "hamburger_menu_update_success.svg"
+    assert (
+        load_svg_spy.call_args_list[1].args[0] == "hamburger_menu_update_available.svg"
+    )
+
+    # Check that new menu entries have been added.
+    menu_actions_after = window.hamburger_button.menu().actions()
+    assert len(menu_actions_after) == 5
+    assert menu_actions_after[2:] == menu_actions_before
+
+    success_action = menu_actions_after[0]
+    assert success_action.text() == "New version available"
+
+    separator = menu_actions_after[1]
+    assert separator.isSeparator()
+
+    # Check that clicking in the new menu entry, opens a dialog.
+    update_dialog_spy = mocker.spy(main_window_module, "UpdateDialog")
+
+    def check_dialog() -> None:
+        dialog = updater.dangerzone.app.activeWindow()
+
+        update_dialog_spy.assert_called_once()
+        kwargs = update_dialog_spy.call_args.kwargs
+        assert "99.9.9" in kwargs["title"]
+        assert "dangerzone.rocks" in kwargs["intro_msg"]
+        assert not kwargs["middle_widget"].toggle_button.isChecked()
+        collapsible_box = kwargs["middle_widget"]
+        text_browser = (
+            collapsible_box.layout().itemAt(1).widget().layout().itemAt(0).widget()
+        )
+        assert collapsible_box.toggle_button.text() == "What's New?"
+        assert text_browser.toPlainText() == "changelog"
+
+        height_initial = dialog.sizeHint().height()
+        width_initial = dialog.sizeHint().width()
+
+        # Collapse the "What's New" section and ensure that the dialog's height
+        # increases.
+        with qtbot.waitSignal(collapsible_box.toggle_animation.finished) as blocker:
+            collapsible_box.toggle_button.click()
+
+        assert dialog.sizeHint().height() > height_initial
+        assert dialog.sizeHint().width() == width_initial
+
+        # Uncollapse the "What's New" section, and ensure that the dialog's height gets
+        # back to the original value.
+        with qtbot.waitSignal(collapsible_box.toggle_animation.finished) as blocker:
+            collapsible_box.toggle_button.click()
+
+        assert dialog.sizeHint().height() == height_initial
+        assert dialog.sizeHint().width() == width_initial
+
+        dialog.close()
+
+    QtCore.QTimer.singleShot(500, check_dialog)
+    success_action.trigger()
+
+    # FIXME: We should check the content of the dialog here.
+
+
+def test_update_error(
+    qtbot: QtBot,
+    updater: UpdaterThread,
+    monkeypatch: MonkeyPatch,
+    mocker: MockerFixture,
+) -> None:
+    """Test that an error during an update check leads to a notification to the user."""
+    # Test 1 - Check that the first error does not notify the user.
+    updater.dangerzone.settings.set("updater_check", True)
+    updater.dangerzone.settings.set("updater_last_check", 0)
+    updater.dangerzone.settings.set("updater_errors", 0)
+
+    # Make requests.get() return an errorthe following dictionary.
+    mocker.patch("dangerzone.gui.updater.requests.get")
+    requests_mock = updater_module.requests.get
+    requests_mock.side_effect = Exception("failed")  # type: ignore [attr-defined]
+
+    window = MainWindow(updater.dangerzone)
+    window.register_update_handler(updater.finished)
+    handle_updates_spy = mocker.spy(window, "handle_updates")
+    load_svg_spy = mocker.spy(window, "load_svg_image")
+
+    menu_actions_before = window.hamburger_button.menu().actions()
 
     with qtbot.waitSignal(updater.finished) as blocker:
         updater.start()
 
-    # The separator and install updates button has been added
-    assert len(window.hamburger_button.menu().actions()) == 5
+    menu_actions_after = window.hamburger_button.menu().actions()
+
+    # Check that the callback function gets an update report.
+    handle_updates_spy.assert_called_once()
+    assert "failed" in handle_updates_spy.call_args.args[0].error
+
+    # Check that the settings have been updated properly.
+    expected_settings = default_updater_settings()
+    expected_settings["updater_check"] = True
+    expected_settings["updater_last_check"] = updater.dangerzone.settings.get(
+        "updater_last_check"
+    )
+    expected_settings["updater_errors"] += 1
+    assert updater.dangerzone.settings.get_updater_settings() == expected_settings
+
+    # Check that the hamburger icon has not changed.
+    assert load_svg_spy.call_count == 0
+
+    # Check that no menu entries have been added.
+    assert menu_actions_before == menu_actions_after
+
+    # Test 2 - Check that the second error does not notify the user either.
+    updater.dangerzone.settings.set("updater_last_check", 0)
+    with qtbot.waitSignal(updater.finished) as blocker:
+        updater.start()
+
+    assert load_svg_spy.call_count == 0
+
+    # Check that the settings have been updated properly.
+    expected_settings["updater_errors"] += 1
     expected_settings["updater_last_check"] = updater.dangerzone.settings.get(
         "updater_last_check"
     )
     assert updater.dangerzone.settings.get_updater_settings() == expected_settings
 
-    # TODO:
-    # 1. Test that hamburger icon changes according to the update status.
-    # 2. Check that the dialogs for new updates / update errors have the expected
-    #    content.
-    # 3. Check that a user can toggle updates from the hamburger menu.
-    # 4. Check that errors are cleared from the settings, whenever we have a successful
-    #    update check.
-    # 5. Check that latest version/changelog, as well as update errors, are cached in
-    #    the settings
+    # Check that no menu entries have been added.
+    assert menu_actions_before == menu_actions_after
+
+    # Test 3 - Check that a third error shows a new menu entry.
+    updater.dangerzone.settings.set("updater_last_check", 0)
+    with qtbot.waitSignal(updater.finished) as blocker:
+        updater.start()
+
+    menu_actions_after = window.hamburger_button.menu().actions()
+    assert len(menu_actions_after) == 5
+    assert menu_actions_after[2:] == menu_actions_before
+
+    error_action = menu_actions_after[0]
+    assert error_action.text() == "Update error"
+
+    separator = menu_actions_after[1]
+    assert separator.isSeparator()
+
+    # Check that clicking in the new menu entry, opens a dialog.
+    update_dialog_spy = mocker.spy(main_window_module, "UpdateDialog")
+
+    def check_dialog() -> None:
+        dialog = updater.dangerzone.app.activeWindow()
+
+        update_dialog_spy.assert_called_once()
+        kwargs = update_dialog_spy.call_args.kwargs
+        assert kwargs["title"] == "Update check error"
+        assert "Something went wrong" in kwargs["intro_msg"]
+        assert "Encountered an exception" in kwargs["middle_widget"].toPlainText()
+        assert "failed" in kwargs["middle_widget"].toPlainText()
+        assert "dangerzone.rocks" in kwargs["epilogue_msg"]
+
+        dialog.close()
+
+    QtCore.QTimer.singleShot(500, check_dialog)
+    error_action.trigger()
 
 
 ##

tests/gui/test_updater.py

@@ -3,9 +3,11 @@ import os
 import platform
 import sys
 import time
+import typing
 from pathlib import Path
 
 import pytest
+from PySide6 import QtCore
 from pytest import MonkeyPatch
 from pytest_mock import MockerFixture
 from pytestqt.qtbot import QtBot
@@ -16,7 +18,7 @@ from dangerzone.gui import updater as updater_module
 from dangerzone.gui.updater import UpdateReport, UpdaterThread
 from dangerzone.util import get_version
 
-from . import generate_isolated_updater, updater
+from . import generate_isolated_updater, qt_updater, updater
 
 
 def default_updater_settings() -> dict:
@@ -191,9 +193,10 @@ def test_update_checks(
     mock_upstream_info = {"tag_name": f"v{get_version()}", "body": "changelog"}
 
     # Make requests.get().json() return the above dictionary.
-    requests_mock = mocker.MagicMock()
-    requests_mock().json.return_value = mock_upstream_info
-    monkeypatch.setattr(updater_module.requests, "get", requests_mock)
+    mocker.patch("dangerzone.gui.updater.requests.get")
+    requests_mock = updater_module.requests.get
+    requests_mock().status_code = 200  # type: ignore [call-arg]
+    requests_mock().json.return_value = mock_upstream_info  # type: ignore [attr-defined, call-arg]
 
     # Always assume that we can perform multiple update checks in a row.
     monkeypatch.setattr(updater, "_should_postpone_update_check", lambda: False)
@@ -211,9 +214,12 @@ def test_update_checks(
     )
 
     # Test 3 - Check that HTTP errors are converted to error reports.
-    requests_mock.side_effect = Exception("failed")
+    requests_mock.side_effect = Exception("failed")  # type: ignore [attr-defined]
     report = updater.check_for_updates()
-    assert_report_equal(report, UpdateReport(error="failed"))
+    error_msg = (
+        f"Encountered an exception while querying {updater.GH_RELEASE_URL}: failed"
+    )
+    assert_report_equal(report, UpdateReport(error=error_msg))
 
     # Test 4 - Check that cached version/changelog info do not trigger an update check.
     updater.dangerzone.settings.set("updater_latest_version", "99.9.9")
@@ -238,6 +244,7 @@ def test_update_checks_cooldown(updater: UpdaterThread, mocker: MockerFixture) -
 
     # # Make requests.get().json() return the version info that we want.
     mock_upstream_info = {"tag_name": "99.9.9", "body": "changelog"}
+    requests_mock().status_code = 200  # type: ignore [call-arg]
     requests_mock().json.return_value = mock_upstream_info  # type: ignore [attr-defined, call-arg]
 
     # Test 1: The first time Dangerzone checks for updates, the cooldown period should
@@ -289,4 +296,146 @@ def test_update_checks_cooldown(updater: UpdaterThread, mocker: MockerFixture) -
     report = updater.check_for_updates()
     assert cooldown_spy.spy_return == False
     assert updater.dangerzone.settings.get("updater_last_check") == curtime
-    assert_report_equal(report, UpdateReport(error="failed"))
+    error_msg = (
+        f"Encountered an exception while querying {updater.GH_RELEASE_URL}: failed"
+    )
+    assert_report_equal(report, UpdateReport(error=error_msg))
+
+
+def test_update_errors(
+    updater: UpdaterThread, monkeypatch: MonkeyPatch, mocker: MockerFixture
+) -> None:
+    """Test update check errors."""
+    # Mock requests.get().
+    mocker.patch("dangerzone.gui.updater.requests.get")
+    requests_mock = updater_module.requests.get
+
+    # Always assume that we can perform multiple update checks in a row.
+    monkeypatch.setattr(updater, "_should_postpone_update_check", lambda: False)
+
+    # Test 1 - Check that request exceptions are being detected as errors.
+    requests_mock.side_effect = Exception("bad url")  # type: ignore [attr-defined]
+    report = updater.check_for_updates()
+    assert report.error is not None
+    assert "bad url" in report.error
+    assert "Encountered an exception" in report.error
+
+    # Test 2 - Check that non HTTP 200 responses are detected as errors.
+    class MockResponse500:
+        status_code = 500
+
+    requests_mock.return_value = MockResponse500()  # type: ignore [attr-defined]
+    requests_mock.side_effect = None  # type: ignore [attr-defined]
+    report = updater.check_for_updates()
+    assert report.error is not None
+    assert "Encountered an HTTP 500 error" in report.error
+
+    # Test 3 - Check that non JSON responses are detected as errors.
+    class MockResponseBadJSON:
+        status_code = 200
+
+        def json(self) -> dict:
+            return json.loads("bad json")
+
+    requests_mock.return_value = MockResponseBadJSON()  # type: ignore [attr-defined]
+    report = updater.check_for_updates()
+    assert report.error is not None
+    assert "Received a non-JSON response" in report.error
+
+    # Test 4 - Check that missing fields in JSON are detected as errors.
+    class MockResponseEmpty:
+        status_code = 200
+
+        def json(self) -> dict:
+            return {}
+
+    requests_mock.return_value = MockResponseEmpty()  # type: ignore [attr-defined]
+    report = updater.check_for_updates()
+    assert report.error is not None
+    assert "Missing required fields in JSON" in report.error
+
+    # Test 5 - Check invalid versions are reported
+    class MockResponseBadVersion:
+        status_code = 200
+
+        def json(self) -> dict:
+            return {"tag_name": "vbad_version", "body": "changelog"}
+
+    requests_mock.return_value = MockResponseBadVersion()  # type: ignore [attr-defined]
+    report = updater.check_for_updates()
+    assert report.error is not None
+    assert "Invalid version" in report.error
+
+    # Test 6 - Check invalid markdown is reported
+    class MockResponseBadMarkdown:
+        status_code = 200
+
+        def json(self) -> dict:
+            return {"tag_name": "v99.9.9", "body": ["bad", "markdown"]}
+
+    requests_mock.return_value = MockResponseBadMarkdown()  # type: ignore [attr-defined]
+    report = updater.check_for_updates()
+    assert report.error is not None
+
+    # Test 7 - Check that a valid response passes.
+    class MockResponseValid:
+        status_code = 200
+
+        def json(self) -> dict:
+            return {"tag_name": "v99.9.9", "body": "changelog"}
+
+    requests_mock.return_value = MockResponseValid()  # type: ignore [attr-defined]
+    report = updater.check_for_updates()
+    assert_report_equal(report, UpdateReport("99.9.9", "<p>changelog</p>"))
+
+
+def test_update_check_prompt(
+    qtbot: QtBot,
+    qt_updater: UpdaterThread,
+    monkeypatch: MonkeyPatch,
+    mocker: MockerFixture,
+) -> None:
+    """Test that the prompt to enable update checks works properly."""
+    # Force Dangerzone to check immediately for updates
+    qt_updater.dangerzone.settings.set("updater_last_check", 0)
+
+    # Test 1 - Check that on the second run of Dangerzone, the user is prompted to
+    # choose if they want to enable update checks. By clicking on, we store this
+    # decision in the settings.
+    def click_ok() -> None:
+        dialog = qt_updater.dangerzone.app.activeWindow()
+        dialog.ok_button.click()  # type: ignore [attr-defined]
+
+    QtCore.QTimer.singleShot(500, click_ok)
+    res = qt_updater.should_check_for_updates()
+
+    assert res == True
+    assert qt_updater.check == True
+
+    # Test 2 - Same as the previous test, but check that clicking on cancel stores the
+    # opposite decision.
+    qt_updater.check = None
+
+    def click_cancel() -> None:
+        dialog = qt_updater.dangerzone.app.activeWindow()
+        dialog.cancel_button.click()  # type: ignore [attr-defined]
+
+    QtCore.QTimer.singleShot(500, click_cancel)
+    res = qt_updater.should_check_for_updates()
+
+    assert res == False
+    assert qt_updater.check == False
+
+    # Test 3 - Same as the previous test, but check that clicking on "X" does not store
+    # any decision.
+    qt_updater.check = None
+
+    def click_x() -> None:
+        dialog = qt_updater.dangerzone.app.activeWindow()
+        dialog.close()
+
+    QtCore.QTimer.singleShot(500, click_x)
+    res = qt_updater.should_check_for_updates()
+
+    assert res == False
+    assert qt_updater.check == None