From a3238eb494c8f3c35c6f423dcc3017902326ff01 Mon Sep 17 00:00:00 2001 From: toofar Date: Fri, 26 Apr 2024 12:04:06 +1200 Subject: [PATCH] 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 not sure if xvfb is used on the Window runs in CI. We could fall back to trying to take screenshots if not running under xvfb but I'm a bit wary of an automatic feature that takes screenshots of people's desktops when running locally. Even if they just to to /tmp/ it might be surprising. We can change it later if it turns out we need to try to take screenshots in more cases. 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 thought 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 as it only supports 24bit, see https://github.com/python-pillow/Pillow/blob/1138ea5370cbda5eb328ec949 8c314d376c81265/src/display.c#L898 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 --- .github/workflows/bleeding.yml | 15 ++++++ .github/workflows/ci.yml | 30 ++++++++++++ .../requirements-tests-bleeding.txt | 1 + misc/requirements/requirements-tests.txt | 1 + misc/requirements/requirements-tests.txt-raw | 1 + pytest.ini | 1 + scripts/dev/changelog_urls.json | 3 +- tests/end2end/fixtures/quteprocess.py | 46 +++++++++++++++++++ tests/end2end/fixtures/test_quteprocess.py | 14 +++++- tox.ini | 1 + 10 files changed, 111 insertions(+), 2 deletions(-) diff --git a/.github/workflows/bleeding.yml b/.github/workflows/bleeding.yml index 47264b2e5..97d95adde 100644 --- a/.github/workflows/bleeding.yml +++ b/.github/workflows/bleeding.yml @@ -42,6 +42,21 @@ jobs: if: "endsWith(matrix.image, '-qt6')" - 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 60f9d12ad..94f561999 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]')" @@ -236,6 +251,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 8514f7730..10c66cc46 100644 --- a/misc/requirements/requirements-tests.txt +++ b/misc/requirements/requirements-tests.txt @@ -35,6 +35,7 @@ packaging==24.1 parse==1.20.2 parse-type==0.6.2 platformdirs==4.2.2 +pillow==10.4.0 pluggy==1.5.0 py-cpuinfo==9.0.0 Pygments==2.18.0 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 6c8d29ff9..0129da083 100644 --- a/pytest.ini +++ b/pytest.ini @@ -85,3 +85,4 @@ filterwarnings = # Python 3.12: https://github.com/ionelmc/pytest-benchmark/issues/240 (fixed but not released) 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 87f5e6f68..cfd82ad06 100644 --- a/scripts/dev/changelog_urls.json +++ b/scripts/dev/changelog_urls.json @@ -168,5 +168,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..0d6edfda7 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,47 @@ 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 + + temp_path = os.environ.get("RUNNER_TEMP", tempfile.gettempdir()) + screenshot_path = pathlib.Path(temp_path) / "pytest-screenshots" + if screenshot_path.exists(): + shutil.rmtree(screenshot_path) + screenshot_path.mkdir() + + self.request.session.stash["screenshot_path"] = screenshot_path + return screenshot_path + + def _take_x11_screenshot_of_failed_test(self): + # Take a basic X11 image grab using pillow. If we want to do something + # fancy like autocropping see pyvirtualdisplay smartdisplay for + # inspiration. + xvfb = self.request.getfixturevalue('xvfb') + if not xvfb: + # Likely we are being run with --no-xvfb + return + + 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 = 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 5afe211b4..607b19fb3 100644 --- a/tox.ini +++ b/tox.ini @@ -31,6 +31,7 @@ passenv = QT_QUICK_BACKEND FORCE_COLOR DBUS_SESSION_BUS_ADDRESS + RUNNER_TEMP HYPOTHESIS_EXAMPLES_DIR basepython = py: {env:PYTHON:python3}