upload e2e failure screenshots as artifacts

This commit takes a screenshot of the active browser window when an
end2end test fails. When running on CI a zip file of screenshots will be
attached to the run summary as an artifact. When run locally screenshots
will be left in /$TMPDIR/pytest-screenshots/.

The screenshot is of the Xvfb screen that the tests are running under.
If there are multiple windows open it will likely only show the active
window because a) we aren't running with a window manager b) the Xvfb
display is, by default, the same size as the browser window.

I'm using pillow ImageGrab, the same as pyvirtualdisplay.smartdisplay. I'm
getting the display number from the pytest-xvfb plugin and formatting it
appropriately (pyvirtualdisplay has an already formatted one which is used by
the smartdisplay, but that's not accessible).

Pillow is now a requirement for running the tests. I though about making
it gracefully not required but I'm not sure how to inform the user with
a warning from pytest, or if they would even want one. Maybe we could
add a config thing to allow not taking screenshots?

I had to bump the colordepth config for pytest-xvfb otherwise pillow
complained that the default 16bit color depth wasn't supported.

I'm saving screenshots to a temp dir because I don't want to put them in
my workdir when running locally. I want to clear the directory for each
run so that you don't get confused by looking at old images. I'm not
100% sure about the lifecycle of the process classes though. Eg if we
have two processes they might both try to create the output directory.
I'm using pytest.session.stash to save the directory so perhaps the
lifecycle of the stash will handle that? Not sure.

Ideally the images would be uploaded somewhere where we could click
through and open them in the browser without having to download a zip
file, but I'm not sure how to achieve that.

It would be nice to print in the test logs that a screenshot was saved
and where to. Just so you could copy paste the filename instead of
having to match the sanitized filename against failing test names. But I
don't know how to log stuff from this stage in the pytest lifecycle.

TODO:
* I'm not sure that the screenshot captures the whole browser window?
  Maybe the browser windows is bigger than the X11 display?

Closes: #7625

10 files changed, 114 insertions, 2 deletions

diff --git a/.github/workflows/bleeding.yml b/.github/workflows/bleeding.yml
index 2587d832b..2f1b52156 100644
--- a/.github/workflows/bleeding.yml
+++ b/.github/workflows/bleeding.yml
@@ -39,6 +39,21 @@ jobs:
         run: "python scripts/dev/ci/problemmatchers.py py3 ${{ runner.temp }}"
       - name: Run tox
         run: dbus-run-session tox -e ${{ matrix.testenv }}
+      - name: Gather info
+        id: info
+        run: |
+            echo "date=$(date +'%Y-%m-%d')" >> "$GITHUB_OUTPUT"
+            echo "sha_short=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT"
+        shell: bash
+        if: failure()
+      - name: Upload screenshots
+        uses: actions/upload-artifact@v4
+        with:
+          name: "end2end-screenshots-${{ steps.info.outputs.date }}-${{ steps.info.outputs.sha_short }}-${{ matrix.image }}"
+          path: |
+            ${{ runner.temp }}/pytest-screenshots/*.png
+          if-no-files-found: ignore
+        if: failure()
   irc:
     timeout-minutes: 2
     continue-on-error: true
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 599ba3b1b..8929f87e4 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -119,6 +119,21 @@ jobs:
         run: "python scripts/dev/ci/problemmatchers.py tests ${{ runner.temp }}"
       - name: Run tox
         run: "dbus-run-session -- tox -e ${{ matrix.testenv }}"
+      - name: Gather info
+        id: info
+        run: |
+            echo "date=$(date +'%Y-%m-%d')" >> "$GITHUB_OUTPUT"
+            echo "sha_short=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT"
+        shell: bash
+        if: failure()
+      - name: Upload screenshots
+        uses: actions/upload-artifact@v4
+        with:
+          name: "end2end-screenshots-${{ steps.info.outputs.date }}-${{ steps.info.outputs.sha_short }}-${{ matrix.image }}"
+          path: |
+            ${{ runner.temp }}/pytest-screenshots/*.png
+          if-no-files-found: ignore
+        if: failure()
 
   tests:
     if: "!contains(github.event.head_commit.message, '[ci skip]')"
@@ -220,6 +235,21 @@ jobs:
         uses: codecov/codecov-action@v3
         with:
           name: "${{ matrix.testenv }}"
+      - name: Gather info
+        id: info
+        run: |
+            echo "date=$(date +'%Y-%m-%d')" >> "$GITHUB_OUTPUT"
+            echo "sha_short=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT"
+        shell: bash
+        if: failure()
+      - name: Upload screenshots
+        uses: actions/upload-artifact@v4
+        with:
+          name: "end2end-screenshots-${{ steps.info.outputs.date }}-${{ steps.info.outputs.sha_short }}-${{ matrix.testenv }}-${{ matrix.os }}"
+          path: |
+            ${{ runner.temp }}/pytest-screenshots/*.png
+          if-no-files-found: ignore
+        if: failure()
 
   codeql:
     if: "!contains(github.event.head_commit.message, '[ci skip]')"
diff --git a/misc/requirements/requirements-tests-bleeding.txt b/misc/requirements/requirements-tests-bleeding.txt
index f1ad30158..10369fc30 100644
--- a/misc/requirements/requirements-tests-bleeding.txt
+++ b/misc/requirements/requirements-tests-bleeding.txt
@@ -20,6 +20,7 @@ git+https://github.com/pygments/pygments.git
 git+https://github.com/pytest-dev/pytest-repeat.git
 git+https://github.com/pytest-dev/pytest-cov.git
 git+https://github.com/The-Compiler/pytest-xvfb.git
+git+https://github.com/python-pillow/Pillow.git
 git+https://github.com/pytest-dev/pytest-xdist.git
 git+https://github.com/john-kurkowski/tldextract
 
diff --git a/misc/requirements/requirements-tests.txt b/misc/requirements/requirements-tests.txt
index 0d8aa9bc4..d888494a9 100644
--- a/misc/requirements/requirements-tests.txt
+++ b/misc/requirements/requirements-tests.txt
@@ -27,6 +27,7 @@ more-itertools==10.2.0
 packaging==24.0
 parse==1.20.1
 parse-type==0.6.2
+pillow==10.3.0
 pluggy==1.4.0
 py-cpuinfo==9.0.0
 Pygments==2.17.2
diff --git a/misc/requirements/requirements-tests.txt-raw b/misc/requirements/requirements-tests.txt-raw
index 54e036106..d37002954 100644
--- a/misc/requirements/requirements-tests.txt-raw
+++ b/misc/requirements/requirements-tests.txt-raw
@@ -25,6 +25,7 @@ pytest-cov
 # To avoid windows from popping up
 pytest-xvfb
 PyVirtualDisplay
+pillow
 # To run on multiple cores with -n
 pytest-xdist
 
diff --git a/pytest.ini b/pytest.ini
index 2de880eae..1ec6a9d4a 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -79,3 +79,4 @@ filterwarnings =
     # Python 3.12: https://github.com/ionelmc/pytest-benchmark/issues/240
     ignore:(datetime\.)?datetime\.utcnow\(\) is deprecated and scheduled for removal in a future version\. Use timezone-aware objects to represent datetimes in UTC. (datetime\.)?datetime\.now\(datetime\.UTC\)\.:DeprecationWarning:pytest_benchmark\.utils
 faulthandler_timeout = 90
+xvfb_colordepth = 24
diff --git a/scripts/dev/changelog_urls.json b/scripts/dev/changelog_urls.json
index 645bb6385..062c9031d 100644
--- a/scripts/dev/changelog_urls.json
+++ b/scripts/dev/changelog_urls.json
@@ -162,5 +162,6 @@
   "mdurl": "https://github.com/executablebooks/mdurl/commits/master",
   "blinker": "https://blinker.readthedocs.io/en/stable/#changes",
   "exceptiongroup": "https://github.com/agronholm/exceptiongroup/blob/main/CHANGES.rst",
-  "nh3": "https://github.com/messense/nh3/commits/main"
+  "nh3": "https://github.com/messense/nh3/commits/main",
+  "pillow": "https://github.com/python-pillow/Pillow/blob/main/CHANGES.rst"
 }
diff --git a/tests/end2end/fixtures/quteprocess.py b/tests/end2end/fixtures/quteprocess.py
index b1e4bbaab..6c491515e 100644
--- a/tests/end2end/fixtures/quteprocess.py
+++ b/tests/end2end/fixtures/quteprocess.py
@@ -5,8 +5,10 @@
 """Fixtures to run qutebrowser in a QProcess and communicate."""
 
 import pathlib
+import os
 import re
 import sys
+import shutil
 import time
 import datetime
 import logging
@@ -18,6 +20,7 @@ import json
 
 import yaml
 import pytest
+from PIL.ImageGrab import grab
 from qutebrowser.qt.core import pyqtSignal, QUrl, QPoint
 from qutebrowser.qt.gui import QImage, QColor
 
@@ -541,6 +544,8 @@ class QuteProc(testprocess.Process):
         except AttributeError:
             pass
         else:
+            if call.failed:
+                self._take_x11_screenshot_of_failed_test()
             if call.failed or hasattr(call, 'wasxfail') or call.skipped:
                 super().after_test()
                 return
@@ -868,6 +873,50 @@ class QuteProc(testprocess.Process):
             self.send_cmd(cmd.format('no-scroll-filtering'))
         self.send_cmd(cmd.format('log-scroll-pos'))
 
+    def _get_x11_screenshot_directory(self):
+        screenshot_path = self.request.session.stash.get("screenshot_path", None)
+        if screenshot_path:
+            return screenshot_path
+
+        screenshot_path = os.path.join(
+            os.environ.get("RUNNER_TEMP", tempfile.gettempdir()),
+            "pytest-screenshots",
+        )
+        if os.path.exists(screenshot_path):
+            shutil.rmtree(screenshot_path)
+        os.mkdir(screenshot_path)
+
+        self.request.session.stash["screenshot_path"] = screenshot_path
+        return screenshot_path
+
+    def _take_x11_screenshot_of_failed_test(self):
+        # See also, pyvirtualdisplay smartdisplay which includes some cropping
+        # logic https://github.com/ponty/PyVirtualDisplay/blob/master/pyvirtualdisplay/smartdisplay.py
+        xvfb = self.request.getfixturevalue('xvfb')
+        if not xvfb:
+            # Likely we are being run with --no-xvfb
+            return
+        # For using IMGrab we must set `xvfb_colordepth = 24` in pytest.ini to override the
+        # default of `16` in pytest-xvfb. Pillow only supports 24bit
+        # https://github.com/python-pillow/Pillow/blob/1138ea5370cbda5eb328ec9498c314d376c81265/src/display.c#L898
+        img = grab(xdisplay=f":{xvfb.display}")
+        current_test = self.request.node.nodeid
+
+        fname = f"{datetime.datetime.now().isoformat()}-{current_test.replace('/', '_')}.png"
+        # upload-artifacts says it doesn't allow these characters if it sees
+        # one of them.
+        bad_chars = '":<>|*?\r\n'
+        for char in bad_chars:
+            fname = fname.replace(char, "_")
+
+        # TODO:
+        # 1. Keep old directories around?
+        # 2. Will different runs in parallel in CI clobber the folder? Might
+        #    have to put them in subdirs with the process ID if so.
+        # 4. Log a "screenshot saved to ..." message?
+        fpath = os.path.join(self._get_x11_screenshot_directory(), fname)
+        img.save(fpath)
+
 
 class YamlLoader(yaml.SafeLoader):
 
diff --git a/tests/end2end/fixtures/test_quteprocess.py b/tests/end2end/fixtures/test_quteprocess.py
index ec0cd55ac..00266fc32 100644
--- a/tests/end2end/fixtures/test_quteprocess.py
+++ b/tests/end2end/fixtures/test_quteprocess.py
@@ -98,8 +98,9 @@ def test_quteproc_error_message(qtbot, quteproc, cmd, request_mock):
         quteproc.after_test()
 
 
-def test_quteproc_error_message_did_fail(qtbot, quteproc, request_mock):
+def test_quteproc_error_message_did_fail(qtbot, quteproc, request_mock, monkeypatch):
     """Make sure the test does not fail on teardown if the main test failed."""
+    monkeypatch.setattr(quteproc, "_take_x11_screenshot_of_failed_test", lambda: None)
     request_mock.node.rep_call.failed = True
     with qtbot.wait_signal(quteproc.got_error):
         quteproc.send_cmd(':message-error test')
@@ -108,6 +109,17 @@ def test_quteproc_error_message_did_fail(qtbot, quteproc, request_mock):
     quteproc.after_test()
 
 
+def test_quteproc_screenshot_on_fail(qtbot, quteproc, request_mock, monkeypatch, mocker):
+    """Make sure we call the method to take a screenshot to test failure."""
+    take_screenshot_spy = mocker.Mock()
+    monkeypatch.setattr(
+        quteproc, "_take_x11_screenshot_of_failed_test", take_screenshot_spy
+    )
+    request_mock.node.rep_call.failed = True
+    quteproc.after_test()
+    take_screenshot_spy.assert_called_once()
+
+
 def test_quteproc_skip_via_js(qtbot, quteproc):
     with pytest.raises(pytest.skip.Exception, match='test'):
         quteproc.send_cmd(':jseval console.log("[SKIP] test");')
diff --git a/tox.ini b/tox.ini
index 31e06e396..b4aada447 100644
--- a/tox.ini
+++ b/tox.ini
@@ -30,6 +30,7 @@ passenv =
     QT_QUICK_BACKEND
     FORCE_COLOR
     DBUS_SESSION_BUS_ADDRESS
+    RUNNER_TEMP
 basepython =
     py: {env:PYTHON:python3}
     py3: {env:PYTHON:python3}