See ee89bd1c39
which was part of PDF.js v4.7.76 (2024-10-06).
This should work both with the old and new version.
(cherry picked from commit 4d2aa13db3)
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.
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.
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
In the Qt6.8.0-beta2 release for some reason the error message now looks like;
Failed to style frame: Failed to read a named property '_qutebrowser' from 'Window': Blocked a frame with origin "http://localhost:35549" from accessing a cross-origin frame.
It seems to have an extra "Failed to read a named property '_qutebrowser' from
'Window'" before the "Blocked a frame ..." bit. Seems like maybe a nested
exception situation? Not sure what's going on there but the exception is still
being caught, which is the point of the test.
Hopefully we don't have more issues with subframes cropping up...
Looks like the kde-unstable arch repo has updated again. It says
6.8.0beta2-1.
I guess the number might change again in the future, still a couple of
months to go before release.
My virtualenv I used to run webkit has rotted long ago and I don't remember
how I set it up. There is a PyQtWebKit project on PyPI but I don't know
who that's published by.
So I figured I would write some notes for myself on using the docker container
used for CI instead. I chose to mount the current directory (which is
presumably a qutebrowser checkout!) directly into the container instead of
cloning it so I could have quicker feedback between making code changes and
running tests.
Then there's a couple of things that stem from that. Since the user in the
container is different from the one in the host we have to move some things
that are normally written to the current directory to be written elsewhere.
There are other ways to approach this (eg you can add `-u $(id -u)` to the
docker command line, although that makes things a bit confusing in the
container) but arguably it's good for the container not to be able to write to
the host, hence making that volume read only.
The TOX_WORK_DIR trick is from
[here](https://github.com/tox-dev/tox/issues/20), apart from with
`{toxinidir}` in it too because the pyroma env was failing with just
`.tox`, saying the pyroma binary needed to be in the allowlist, possibly
it was doing full path matching without normalizing.
The hypothesis folks
[here](https://github.com/HypothesisWorks/hypothesis/issues/2367#issuecomment-595524571)
say if you want to override the examples DB location with an env var to
do it yourself. It's actually only a warning from hypothesis, it says it
falls back to an in-memory DB, but I guess the tests run with
warnings-are-errors. You can also pass `database=None` to make
hypothesis skip example storage altogether.
I'm using tox to run commands in a virtualenv with the right stuff in it
because, uh, because I was copying the CI workflow actually. I just found out
about the `exec` subcommand to override the `commands` defined for the env,
neat! One point of awkwardness about that is that since we are using the
PyQt from the OS we need any virtualenv we use to have access to the OS
packages, which isn't the default for virtualenvs created by tox. The
text envs use the link_pyqt script for that but if you are using this
container and the first thing you do is run `tox exec` then that
wouldn't have been run. So I'm setting `VIRTUALENV_SYSTEM_SITE_PACKAGES`
to tell tox to always make the system packages available in the
virtualenvs it manages.
I did try using the mkvenv script instead of tox but it complained when
trying to install the current directory in editable mode because
setup.py tries to write to a git-commit-id file.
The "Async question interrupted by async one" test was failing on CI but
not locally. Not sure why, changing the tests to skip instead of xfail.
The downside is that we won't bet a notification when upstream fixes the
issues, hopefully they mark the QTBUG as closed and we see it that way.
I think we do have to do something else to deal with this persistent
permission thing anyway, assuming they don't change it to be
off-by-default, so I'm sure we'll be looking in this area again!
They'll at the very least be re-enabled when we get a PyQt 6.8.
WebEngine now has it's own mechanism to remember permission, and it's
turned on by default. We can't disable it until PyQt picks up the new
`QWebEngineProfile::setPersistentPermissionsPolicy()`. So the first test
that prompts for a permission will persist that setting and later ones
will fail because they don't get the prompt they expect.
For now lets set them to xfail while we figure out what to do with
permission persisting for actual users. That we we can reduce the noise
in the test results!
The WebEngine permissions persistence mechanism doesn't seem to
respect whatever we are doing to separate storage per-basedir. Testing
with a temp basedir like so:
python3 -m qutebrowser -T https://web-push-book.gauntface.com/demos/notification-examples/
I see this file has been created under my home directory:
$ cat ~/.local/share/qutebrowser/qutebrowser/QtWebEngine/Default/permissions.json
{"Notifications":{"https://web-push-book.gauntface.com/":true}}
I've raised an issue upstream about that here: https://bugreports.qt.io/browse/QTBUG-126595
We've got two things to think about regarding how to deal with this new
on-by-default feature:
1. what do we do for the tests? We can Disable the feature (if on new
enough PyQt) or add a test setup step that ... restarts the browser
and deletes the permissions.json. That's not great
2. what do we do for real users? See below
By default I would recommend disabling the webengine one since we
already have our own. BUT we can't actually disable it until PyQt
updates with the new APIs, which can take a while, and it's pretty
likely people will be using the new Qt version before PyQt updates. So
it would be best to figure out what we can do before that! Can we make
it respect the basedir data path? Can we make it write to some fake file
we throwaway? chmod +i?
Hopefully Qt makes the permission JSON respect the data path we set and
then at the very least we can remove the JSON file after a permission is
set. It'll still be a change in behavior for users on Qt 6.8 and PyQt
6.7 though as it'll likely remember permissions within a browser
instance by default, which isn't the case for our implementation
currently.
Related to: https://github.com/qutebrowser/qutebrowser/issues/8242#issuecomment-2175949686
Background:
As part of my work on reproducible builds for openSUSE, I check that software still gives identical build results in the future.
The usual offset is +16 years, because that is how long I expect some software will be used in some places.
This showed up failing tests in our package build.
See https://reproducible-builds.org/ for why this matters.
We want to make sure that the selection model gets deleted when clearing
the model, since we are switching from doing that directly to having it
happen indirectly based off of signals we don't manage.
Hopefully this doesn't end up to be flaky. I think we are depending on
this happening in two different Qt even loop runs (`model.deleteLater()`
in the first one then `selmod.deleteLater()` gets called from the
`model.deleted` signal).
I tried using `qtbot.wait_signals()` with a list but it segfaulted in
some cleanup method. Perhaps is doesn't handle the deleted signal well?
Alternately we could maybe just wait for the selmodel one and check
`sip.isdelted(model)` to check the model is gone too.
- Drop trailing comma inside trivial tuple
- Use r"""...""" for string containing ", as \" inside r"..." is taken literally
(I'm surprised it works!)
- Use ['"] instead of ('|")
- Also adjust the inner [^'] to [^'"] for consistency
The `test_cleanup()` test for guiprocess was triggering the early timer
warning messages because it was using a VeryCourse timer with a 100ms
interval. Changed it to to a normal Course timer (the default) for that
test.
For the `ipc-timeout` timer we know that is happening in the tests on
windows. It's being logged in lost of e2e tests, the elapsed times it's
logging are between 0 and 0.020s. I'm not sure if it's the right thing
to be changing the log level in production code or marking the messages
as expected in test code.
Since we added some sanity checking in usertypes.Timer() around
QTBUG-124496 it would be convenient if there was a reminder for future
timer users to use our Timer object instead. Here's one!
It's looking for QTimer initialisations, we are still allowing
QTimer.singleShot(), although that probably can hit the same issue.
It uses an end-of-line anchor in the regex so you can put a comment (any
comment) on the end of the line to ignore the check.
- Add a couple new "raise utils.Unreachable" to avoid
possibly-used-before-assignment issues.
- Simplify an "if" for the same reason
- Remove an unneeded "return"
- Use "NoReturn" to prepare for pylint knowing about it in the future:
https://github.com/pylint-dev/pylint/issues/9674
- Add some ignores for used-before-assignment false-positives
- Ignore new undefined-variable messages for Qt wrapers
- Ignore a new no-member warning for KeySequence:
https://github.com/pylint-dev/astroid/issues/2448#issuecomment-2130124755
Change `CompletionView.on_clear_completion_selection()` to call the Qt
selection model getter, instead of our one. Since it can be called when the
selection model has already been cleared and our one asserts that there
is a selection model to return.
Back in the distant past there was a change to handle the completion widget's
selection model being None when the `on_clear_completion_selection()` slot was
called: https://github.com/qutebrowser/qutebrowser/commit/88b522fa167e2f97b
More recently a common getter for the selection model was added so we could
have a single place to apply type narrowing to the returned object from the Qt
getter (the type hints had been updated to be wrapped in `Optional`): https://github.com/qutebrowser/qutebrowser/commit/92dea988c01e745#diff-1559d42e246323bea35fa064d54d48c990999aaf4c732b09ccd448f994da74cf
It seems this is one place where it does, and already did, handle that
optional. So it didn't need to change to use the new getter. This is called
from the `Command.on_mode_left` signal, not sure why the selection model is
None here. Perhaps it already gets cleared by the effects of the `hide_cmd`
signal that gets fired earlier, or perhaps even from the `self.hide()` on the
line before. Anyway, this was working for several years and seems harmless
enough.
I was getting crash reports from someone about this. Not sure what's going wrong
there (hence the additional information in the exception).
What's clear however is that we're raising ParseError, but only handling that
when actually parsing. The code calling copy_/_find_webengine_resources only
handles OSError. So let's raise a FileNotFoundError instead.
We already had all this information in a comment anyways.
I made it machine-readable using:
s/#\s+(\d*)\.(\d*)\.(\d*): Security fixes up to ([^ ]*)\s+\((.*)\)/utils.VersionNumber(\1, \2, \3): (_BASES[XX], '\4'), # \5
plus some manual post-processing.
Thanks to that, we can now get the security version from that data even on
QtWebEngine < 6.3, if that information is known. When we fall back to a base
version (e.g. 6.7.99 -> 6.7), we make sure to not pretend that we have the .0
state of things, though.
Finally, we cross-check the information against the current Qt version if we
have the API, which mostly ensures the data is accurate for human readers.
See #7187 and #8139.
More readable now that we have more information in it.
Also always show the source, now that we have the space for it, and "UA" isn't
the obvious default anymore anyways.
Similarly to 24d01ad257, failing Qt 5.15 tests
showed some evidence of us being stuck in command mode in the next test file
(hints.feature). On the first test there ("Scenario: Using :hint-follow outside
of hint mode (issue 1105)"):
17:38:51.073 ERROR message message:error:63 hint-follow: This command
is only allowed in hint mode, not command.
but:
end2end.fixtures.testprocess.WaitForTimeout: Timed out after 15000ms waiting
for {'category': 'message', 'loglevel': 40, 'message': 'hint-follow: This
command is only allowed in hint mode, not normal.'}.
I agree with what has been said: This should never happen, because we restart
the qutebrowser process between test files. I did some of the mentioned "more
examination" but also don't have an explanation.
To avoid more flaky tests, let's roll with another bandaid solution.
Florian Bruhin
toofar
Bernhard M. Wiedemann
Evan Chen