I find it difficult to reproduce issues on old python versions, or when
archlinux brings in a newer build of Qt than I have and I can't be
bothered rebuilding.
I've been using docker for a while to help test in these environments
locally, but the docker command is a bit long and unwieldy. Docker
compose is a method I've used before to provide canned docker
invocation. We could probably get away with writing our own script for
it, but using standard components is good. This should work with
podman-compose too.
This commit encodes the docker command I've been using so that I, and
other people, can use it with a less arcane command line. Hopefully it
helps other people. There are still a few rough edges like for any
runs of the "other" service you will pretty much have to drop into a
bash shell and install tox and some X11 stuff. But the images we publish
work in one-liners (although they would be a little faster with a
tox/pip cache directory mounted from the host so they don't setup the
venv every time).
See also: the docker related hints in the contributor guidlines.
This flag is vital for the allow-list configuration to be picked up.
It should be set globally and `dev-qt/qtwebengine` should be
recompiled after it's enabled.
Ref #8313.
See 433074c681, this is the same cause. An older version of a
package being included in requirements files because setuptools injects
its vendored packages into sys.path and we use pip freeze to build lock
files. Then when you install two requirements files at the same time they
end up having conflicting versions. This at least means we include the
latest version, which will do until we move to a method of generating
lock files that just works off of the raw requirements file.
Qt have updated their permission storage feature so it respects our
the setting our basedir feature uses, so now all the tests that use
"Given I have a fresh instance" are passing.
The remaining failing ones do pass if I make them run in a fresh
instance, but I am leaving them as xfail because a) opening a new
instance is slow b) the new upstream behaviour causes a regression in
the qutebrowser behavior (you don't get re-prompted where you would have
been previously) so I feel like it is correct for some tests to be
failing! We have to set AskEveryTime at some point and we can address
them then.
The message is:
The following paths were searched for Qt WebEngine dictionaries:
/tmp/qutebrowser-basedir-qrhbqblr/data/qtwebengine_dictionaries
but could not find it.
Spellchecking can not be enabled.
Tests are failing on "Logged unexpected errors".
Currently the dependency update job is failing[1] because one of the
tests installs multiple requirements files before running the tests and
it claims they have conflicting versions of `importlib_resources` (6.4.0
vs 6.4.4). 6.4.0 is in the pinned files and there is a 6.4.4 available.
Looking though the logs the first time I see importlib_resources==6.4.0
is when printing the requirements for the `test` requirements file.
But it's not mentioned at all when installing that file. Which makes me
think it found it's way into the virtualenv by some other means.
Looking at git blame for the test requirements lock file, it looks like
importlib_resources was introduced in
https://github.com/qutebrowser/qutebrowser/pull/8269 and indeed I can
see version 6.4.0 in setuptools vendored folder[2].
So it looks like this is another issue caused by setuptools adding their
vendored packages into sys.path.
Options I can see for resolving this:
a. add importlib_resources as a dependency in requirements.txt-raw so
that we always pull down the newest one, even though we don't need it
b. add an @ignore line for importlib_resources
* I think in the unlikely event we end up needing it then it being
ignored might be hard to spot
c. drop python 3.8 support
d. switch to a requirements compilation method that doesn't use `pip
freeze`
I've chosen (a) here because I think it's less surprising than (b), less
work than (c) and I already have a PR up for (d). And it's only pulled
down for 3.8 anyhow, so we'll drop this workaround when we drop that.
[1]: https://github.com/qutebrowser/qutebrowser/actions/runs/10660624684/job/29544897516
[2]: https://github.com/pypa/setuptools/tree/main/setuptools/_vendor
This bit is printed right about the test result summary, so now that the
file names are just test names, printing them out just above the full
test paths in the results seems a bit redundant.
The section header prints out the file path with the screenshots and
that's the important part. It looks fine to me printing a section header
without any section contents. Example:
-------------------- End2end screenshots available in: /tmp/pytest-of-user/pytest-108/pytest-screenshots ---------------------
=================================================== short test summary info ====================================================
FAILED tests/end2end/features/test_completion_bdd.py::test_deleting_an_ornpen_tab_via_the_completion - AssertionError: assert 'http://local...ata/hello.txt' == 'http://local...ata/sello.txt'
FAILED tests/unit/utils/test_resources.py::TestReadFile::test_glob_deleting_resources_subdir[True-pathlib] - AssertionError: assert ['html/subdir...ir-file.html'] == ['html/subdir...ir-sile.html']
FAILED tests/unit/utils/test_resources.py::TestReadFile::test_glob_deleting_resources_subdir[False-zipfile] - AssertionError: assert ['html/subdir...ir-file.html'] == ['html/subdir...ir-sile.html']
FAILED tests/unit/utils/test_resources.py::TestReadFile::test_glob_deleting_resources_subdir[True-zipfile] - AssertionError: assert ['html/subdir...ir-file.html'] == ['html/subdir...ir-sile.html']
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/unit/utils/test_resources.py::TestReadFile::test_glob_deleting_resources_subdir[False-pathlib] - AssertionError: assert ['html/subdir...ir-file.html'] == ['html/subdir...ir-sile.html']
=========================================== 6 failed, 23 passed, 8 skipped in 22.59s ===========================================
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.
The upload artifact action can't collect artifacts from /tmp/ in the test
runners. So now that we are writing the screenshots that we want to collect
later to the pytest `tmp_path` location we need to make sure that lives
somewhere the later actions can find it.
Pytest uses `tempfile.gettempdir()` to find the temp dir, and that respects a
number of environment variables including `TMPDIR`. This commits sets TMPDIR
to RUNNER_TEMP in in our test runners to make pytest uses the temp dir that's
mounted into the action containers.
For the docker based runners I can use the `env` map, but for the ubuntu ones
it didn't let me expand `${{ runner.temp }}` in the end map under `step`, so
I'm writing it to the env file for the runner instead. It failed to parse the
action yaml and said:
> Unrecognized named-value: 'runner'. Located at position 1 within expression: runner.temp
For the user part of the `pytest-of-$user` directory, I looked at the new
screenshot related test summary lines to see what the user was called.
`runner` on the ubuntu containers and `user` in our docker containers. Pytest
maintains the "pytest-current" symlink to the latest temp folder.
On GitHub the RUNNER_TEMP dir is inside the user's home directory. I
think the spirit of the check is making sure you aren't touching stuff
like ~/.config/qutebrowser/ in the tests, if it's within a specified
tempdir it should be fine
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
Starting with PyInstaller 6.10 (6.9?), we're supposed to tell PyInstaller when
we restart our application (and a subprocess should outlive this process).
In their words:
The above requirement was introduced in PyInstaller 6.9, which changed the
way the bootloader treats a process spawned via the same executable as its
parent process. Whereas previously the default assumption was that it is
running a new instance of (the same) program, the new assumption is that the
spawned process is some sort of a worker subprocess that can reuse the
already-unpacked resources. This change was done because the worker-process
scenarios are more common, and more difficult to explicitly accommodate
across various multiprocessing frameworks and other code that spawns worker
processes via sys.executable.
https://pyinstaller.org/en/stable/common-issues-and-pitfalls.html#independent-subprocesshttps://pyinstaller.org/en/stable/CHANGES.html (6.10)
While from a quick test on Windows, things still worked without setting the
variable (possibly because we don't use a onefile build), it still seems
reasonable to do what PyInstaller recommends doing.
Follow-up to #8269.
toofar
Florian Bruhin
qutebrowser bot
Sviatoslav Sydorenko