Add some basic tests for SignalHandler

It doesn't look like this class has any unit tests currently. And since
we are adding a new signal handler to it I took the opportunity to add a
few tests to it to establish a bit of a framework so that next time we
touch it it will be easier to add more.

I didn't go for full coverage here. It's an improvement over what was
there previously and it should make merging more palatable. I don't
think handling SIGHUP is a very risky feature.

I chose to use mocker.patch over monkeypatch.setattr because I think it
provides a more friendly and powerful API due to being more tightly
integrated with magic mocks. And with `mocker.patch.object` you can even
use actual object names instead of strings, the same as monkeypatch
allows.

One thing I'm less confident with here is mocking all multiple things in
a couple of fixtures. Writing one fixture for each little thing I mock
doesn't feel appealing to me right now, but for mocks that tests need to
access there isn't really any idiomatic way to let the tests access
them. I previously just had the one fixture but pulled out the read
config one for that purpose. It's a pattern I'll have to think on a bit
I think. Probably having to have developers thing about the balance of
boilerplate vs accessibility is cognitive load that we want to avoid.
Hmm.

Anyway, here are the options I was looking at to let test code access
mocks that where all shoved away into the one fixture:

1. group the tests into a class and put the mocks in class variables:
   could work fine but I don't think it's very idiomatic for pytest?
2. return some kind of meta object from the fixture that has the object
   under test as one attribute and the mocks as other ones: hmm,
   returning multiple values from a method always seemed a bit smelly to
   me
3. make one fixture for each of the mocks, have the main fixture depend
   on them all, tests that need to modify them can depend on them too:
   writing all those fixtures seems tedious and I don't think it would
   be a clear win in terms of readability. *sigh*, I suppose I should
   pull the ones I'm modifying out at least so other people don't copy
   my lazyness in the future
4. have the test code access the mocks from the real code, eg
   `configfiles.sys.exit.side_effect = ...`: this works fine but I feel
   like it's relying on an implementation detail and probably shouldn't
   be encouraged? Not sure

In case anyone is struggling with git blame and wondering what the
QSocketNotifier stuff is about, this commit should explain it: 210ce8ca7c

qutebrowser/misc/crashsignal.py

@@ -21,9 +21,8 @@ from qutebrowser.qt.core import (pyqtSlot, qInstallMessageHandler, QObject,
                           QSocketNotifier, QTimer, QUrl)
 from qutebrowser.qt.widgets import QApplication
 
-from qutebrowser.config import configfiles, configexc
-
 from qutebrowser.api import cmdutils
+from qutebrowser.config import configfiles, configexc
 from qutebrowser.misc import earlyinit, crashdialog, ipc, objects
 from qutebrowser.utils import usertypes, standarddir, log, objreg, debug, utils, message
 from qutebrowser.qt import sip
@@ -324,6 +323,12 @@ class SignalHandler(QObject):
         self._activated = False
         self._orig_wakeup_fd: Optional[int] = None
 
+        self._handlers = {
+            signal.SIGINT: self.interrupt,
+            signal.SIGTERM: self.interrupt,
+            signal.SIGHUP: self.reload_config,
+        }
+
     def activate(self):
         """Set up signal handlers.
 
@@ -333,12 +338,8 @@ class SignalHandler(QObject):
         On Unix, it uses a QSocketNotifier with os.set_wakeup_fd to get
         notified.
         """
-        self._orig_handlers[signal.SIGINT] = signal.signal(
-            signal.SIGINT, self.interrupt)
-        self._orig_handlers[signal.SIGTERM] = signal.signal(
-            signal.SIGTERM, self.interrupt)
-        self._orig_handlers[signal.SIGHUP] = signal.signal(
-            signal.SIGHUP, self.reload_config)
+        for sig, handler in self._handlers.items():
+            self._orig_handlers[sig] = signal.signal(sig, handler)
 
         if utils.is_posix and hasattr(signal, 'set_wakeup_fd'):
             # pylint: disable=import-error,no-member,useless-suppression

tests/unit/misc/test_crashsignal.py

@@ -0,0 +1,102 @@
+# SPDX-FileCopyrightText: Florian Bruhin (The Compiler) <mail@qutebrowser.org>
+#
+# SPDX-License-Identifier: GPL-3.0-or-later
+
+"""Tests for qutebrowser.misc.crashsignal."""
+
+import signal
+
+import pytest
+
+from qutebrowser.config import configexc
+from qutebrowser.qt.widgets import QApplication
+from qutebrowser.misc import crashsignal, quitter
+
+
+@pytest.fixture
+def read_config_mock(mocker):
+    # covers reload_config
+    mocker.patch.object(
+        crashsignal.standarddir,
+        "config_py",
+        return_value="config.py-unittest",
+    )
+    return mocker.patch.object(
+        crashsignal.configfiles,
+        "read_config_py",
+        autospec=True,
+    )
+
+
+@pytest.fixture
+def signal_handler(qtbot, mocker, read_config_mock):
+    """Signal handler instance with all external methods mocked out."""
+    # covers init
+    mocker.patch.object(crashsignal.sys, "exit", autospec=True)
+    signal_handler = crashsignal.SignalHandler(
+        app=mocker.Mock(spec=QApplication),
+        quitter=mocker.Mock(spec=quitter.Quitter),
+    )
+
+    return signal_handler
+
+
+def test_handlers_registered(signal_handler):
+    signal_handler.activate()
+
+    for sig, handler in signal_handler._handlers.items():
+        registered = signal.signal(sig, signal.SIG_DFL)
+        assert registered == handler
+
+
+def test_handlers_deregistered(signal_handler):
+    known_handler = lambda *_args: None
+    for sig in signal_handler._handlers:
+        signal.signal(sig, known_handler)
+
+    signal_handler.activate()
+    signal_handler.deactivate()
+
+    for sig in signal_handler._handlers:
+        registered = signal.signal(sig, signal.SIG_DFL)
+        assert registered == known_handler
+
+
+def test_interrupt_repeatedly(signal_handler):
+    signal_handler.activate()
+    test_signal = signal.SIGINT
+
+    expected_handlers = [
+        signal_handler.interrupt,
+        signal_handler.interrupt_forcefully,
+        signal_handler.interrupt_really_forcefully,
+    ]
+
+    # Call the SIGINT handler multiple times and make sure it calls the
+    # expected sequence of functions.
+    for expected in expected_handlers:
+        registered = signal.signal(test_signal, signal.SIG_DFL)
+        assert registered == expected
+        expected(test_signal, None)
+
+
+def test_reload_config_call_on_hup(signal_handler, read_config_mock):
+    signal_handler._handlers[signal.SIGHUP](None, None)
+
+    read_config_mock.assert_called_once_with("config.py-unittest")
+
+
+def test_reload_config_displays_errors(signal_handler, read_config_mock, mocker):
+    read_config_mock.side_effect = configexc.ConfigFileErrors(
+        "config.py",
+        [
+            configexc.ConfigErrorDesc("no config.py", ValueError("asdf"))
+        ]
+    )
+    message_mock = mocker.patch.object(crashsignal.message, "error")
+
+    signal_handler._handlers[signal.SIGHUP](None, None)
+
+    message_mock.assert_called_once_with(
+        "Errors occurred while reading config.py:\n  no config.py: asdf"
+    )