Not yet quite sure what exactly is the culprit, but this seems to help for all
tests (!) to pass with Xvfb locally.
For now only scoped to Qt 6.9.0. Will probably already need to reevaluate with
the RC, but definitely with the final release.
See #8444
Hopefully now that we have reporting in the test results, and pytest
retaining of old directories, we don't have to encode so much
information in the filenames to help you make sense of them.
Previously the png filenames looked like this:
2024-08-24T12_42_11.160556-tests_end2end_features_test_completion_bdd.py__test_deleting_an_open_tab_via_the_completion.png
Now they just have the individual test name, eg:
test_deleting_an_open_tab_via_the_completion.png
The two times people will want to look at these files and I want to make
sure they can find what they are looking for are:
* running the tests locally
* the directory with the images is printed out right above the
pytest summary, hopefully that is a clear enough reference to the
tests and that has the full path to the tests, not just the name
* if people run multiple test runs and want to find older images
they will have to know, or guess, how the pytest temp dir naming
scheme works, or go back in their terminal scrollback
* when downloading images as artifacts to debug tests
* The zip files with images from a job currently have names like
end2end-screenshots-2024-08-18-fef13d4-py310-pyqt65-ubuntu-22.04.zip
* Hopefully that zip file name is specific enough
* I'm not sure if the individual filenames with just test name in
them are specific enough for this case. But presumably people will
be looking at the run logs in CI anywhere and will be able to
match up a failing test with the screenshot easy enough
Pytest appears to sanitize test names enough for upload-artifact.
Couldn't see any docs on it, but I put all the characters it complains
about in a BDD test name and they all go stripped out.
I would like it to be obvious to contributors who run the tests locally
that there are screenshots of the processes under test that they can
examine. I don't think it's obvious that there could be useful files
sitting round in a temp directory.
This commit adds the screenshot file paths to a user property on failed
tests then adds a custom report section that pulls that lists those
properties. That way when there is errors users will get the paths to
the images printed out alongside the report of failed tests.
I find it difficult to navigate the internals of pytest. I tried various
ways of printing information and getting that information to methods
that could do the printing but couldn't get anything to work. I ended up
entirely copying this SO post which worked really well for attaching
information to test results in a place that is accessable to the
reporting hook: https://stackoverflow.com/a/64822668
It's added to the end of the existing terminal report hook, because
while it seems you can have two of those hooks, things can get pretty
confusing with interleaved reports and not all of them running every
time.
--------------------- End2end screenshots available in: /tmp/pytest-of-user/pytest-56/pytest-screenshots ---------------------
2024-08-17T14_49_35.896940-tests_end2end_features_test_utilcmds_bdd.py__test_cmdrepeatlast_with_modeswitching_command_deleting.png
2024-08-17T14_49_37.391229-tests_end2end_features_test_completion_bdd.py__test_deleting_an_open_tab_via_the_completion.png
=================================================== short test summary info ====================================================
FAILED tests/end2end/features/test_utilcmds_bdd.py::test_cmdrepeatlast_with_modeswitching_command_deleting - AssertionError: assert 'http://local...ata/hello.txt' == 'http://local...ata/sello.txt'
FAILED tests/end2end/features/test_completion_bdd.py::test_deleting_an_open_tab_via_the_completion - AssertionError: assert 'http://local...ata/hello.txt' == 'http://local...ata/sello.txt'
====================================================== 2 failed in 5.18s =======================================================
From adding debug messages I can see:
RUNNER_TEMP=/home/runner/work/_temp
/tmp/pytest-of-runner/pytest-0/pytest-screenshots
Means that I don't think GHA will be able to collect the temp files
because they are not being written to the temp dir being mounted in from
outside. I think that's the case anyway. Might have to pass
--basetemp=$RUNNER_TEMP to pytest or set TEMP or something. TODO
Saving screenshots to the temp directories managed by pytest means we
don't have to worry about cleaning up from previous runs because pytest
will create a new folder for each run. Now that we aren't cleaning stuff
up means we don't have to worry about workers clobbering each other
because all they are going to do is write to files with the names of
tests which have already been distributed amongst them.
Moving to the pytest temp dirs instead of a hardcoded one also means
that it'll be less obvious to users where the screenshots are. Pytest
doesn't seem to have much UX around pointing people to interesting
artifacts in the "temp" dir. So I'll have another look at adding this
information to the test report.
Since this implementation is now more tightly couple with pytest I've
pulled some code out of the QuteProc process into fixtures.
TODO:
* add screenshot locations to test report
* adapt GHA zip file creation to get files from /tmp/pytest-of-$user/pytest-current/pytest-screenshots
* review filenames to see if pytest does a good enough sanitization for
us, from what I've seen it doesn't slugify that path of the tests, and
it tends to truncate names. I think having the full test path in the
filenames could be useful for people who download the zip file from
the github actions to investigate CI failures
Ohhh! I didn't realize the e2e tests where running in parallel already.
Interesting.
Anyhow, use the builtin `filelock` module to make sure different test
processes in the same session don't re-create the screenshot directory.
This is based on advice here: https://pytest-xdist.readthedocs.io/en/latest/how-to.html#making-session-scoped-fixtures-execute-only-once
I'm saving the lock object to the session stash because it seems the
lock is released when the FileLock object is destroyed. So this ties
it's lifecycle to the test session lifecycle, with xdist hopefully all
the tests processes live for the whole run.
When taking screenshots of the test process running under xvfb it's
offset from the top left corner, the default geometry of qutebrowser is
800x600+50+50. The default size of pytest-xvfb is 800x600, which means
part of the browser window is outside the X11 screen and doesn't get
captured.
This commit duplicates the width and height from the default geometry in
mainwindow.py but sets the x and y offsets to zero so that the browser
window is fully contained within the X11 window.
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
mypy:
Mypy knows about the QDataStream.Status.SizeLimitExceeded attribute now,
so we can remove the ignore. But mypy for pyqt5 doesn't know about it,
so put the whole graceful block behind an additional conditional as well
to hide it from pyqt5 mypy.
cheroot:
looks like the format of the error message we are already ignoring
changed slightly. The `ssl/tls` bit changed to `sslv3`, at least in our
setup.
This only shows up on the webkit CI job.
Probably something to do with the fact that we are using webkit builds
from the distant archives. I'm sure it'll break for real one of these
days.
As shown in detail by toofar in dff25d10bc,
mesa 23.3 seems to trigger some new error messages.
On my setup locally, it also triggers the warning that's ignored here.
Why that's not the case on CI or for others is unclear to me, I gave up
trying to understand graphics stuff on Linux.
I'll just presume this is the same underlying cause, and refer to the
commit message of the commit above for more details.
Mesa upgraded from 23.2.1-2 to 23.3.1-1 a couple of days ago. Since then
there has been some non-fatal InvalidLine errors in the CI jobs (eg
https://github.com/qutebrowser/qutebrowser/actions/runs/7281982007/job/19843511920)
Based on https://gitlab.freedesktop.org/mesa/mesa/-/issues/10293 I'm
assuming these log messages don't actually indicate errors and the tests
pass when they are ignore.
Weirdly, I'm only seeing these errors related to the
`tests/end2end/test_invocations.py::test_misconfigured_user_dirs` test,
I'm not sure why. It doesn't look much different from the ones around
it.
Possibly we could ignore these lines just for that test, or maybe play
round with it more to find out why it is different. But since the lines
are non fatal I'll just ignore them everywhere. I'm a little worried
about that because they are quite generic, but at least they are still
logged when they are ignored.
They might change these error logs messages to be warning log messages
based on that issue. But it'll probably still fail the tests since we
match on all log lines?
The "always try zink" change was introduced in https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25640
It looks like there might be a `LIBGL_KOPPER_DISABLE` which may skip
this behaviour. Not I'm not sure, the commit message says:
> don't load non-sw zink without dri3 support
> this is going to be broken, so don't bother trying
> also add LIBGL_KOPPER_DRI2 so people can continue to footgun if they
> really really want to
I assume we aren't trying to run with non-sw zink but with non-zink sw?
idk
Maybe we should be setting force_software_rendering=software-opengl or
LIBGL_ALWAYS_SOFTWARE instead so that it never tries to use the vulkan
backed zink?
This is required to use BDD steps like the following:
```
When I open file://path/to/file.html
```
If we don't treat `file:` as a special scheme, we implicitly convert it to an invalid
URL:
```
http://localhost:48595/file:///path/to/file.html
```
See 5ff0a573f4
With that commit, encoding the header ourselves means that we'll actually be
navigating to the path `/b'data/...'` instead of `/data/...`.
We're deprecating vim modelines in favor of `.editorconfig`.
Removing vim modelines could be done using two one-liners. Most of the vim modelines
were followed by an empty line, so this one-liner took care of these ones:
```sh
rg '^# vim: .+\n\n' -l | xargs sed -i '/^# vim: /,+1d'
```
Then some of the vim modelines were followed by a pylint configuration line, so running
this one-liner afterwards took care of that:
```sh
rg '^# vim:' -l | xargs sed -i '/^# vim: /d'
```
This was first introduced in adbdfcbad3,
most likely because we got logging from the built-in Werkzeug webserver.
It doesn't seem to be needed anymore, possibly since
41c4ee3e2f where we started using CherryPy
for the SSL server too.
This should fix nighly bleeding tests, because the before_first_request
decorator got removed in Flask:
https://github.com/pallets/flask/pull/4621https://github.com/pallets/flask/pull/4995
Florian Bruhin
toofar
Philipp Albrecht