- 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
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.
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.
Previously (a209c86c55) I've added a delay in browser code before sending
events to the page to account with a race condition where events weren't
processed after navigating. Then I had to add extra checks to tests that had
tight timing requirements.
That was for click-element, this commit reverts an attempt at the same
strategy for fake-key and instead adds wait statements in the tests that where
hitting the original race condition (sending events "too soon" after a
navigation).
The reason for the different approach here is that after adding the delay in
fake-key I had to add an extra "wait for log line" message to a few tests with
tight timing requirements to watch for a Tab key press. That worked great for
webengine but it made some tests start failing on webkit. We don't seem to get
that log message on webkit. I've got no-idea why and frankly think I've spent
way too much time on just a handful of tests already.
It's unfortunate we have to add manually delays in the e2e tests. It makes me
think if anyone else adds a test in the future with this combination of steps
(open page, run fake-key) they'll run into the same issue and it'll be hard to
spot. Oh well, we'll deal with that when it comes up.
The tests that where failing on webkit are the ones in caret.feature
touched in this commit.
Reverts included in this commit:
Revert "Delay fake-key events by 10ms"
This reverts commit 9f050c7460.
Revert "Wait for evidence of Tab key press in tests before proceeding"
This reverts commit d47d247941.
In the previous commit 9f050c7460 "Delay fake-key events by 10ms" I delayed
events for fake-key by 10ms to deal with a timing issue with sending events
right after a navigation not being processed.
Just like last time that's caused a few tests with tight timing constraints to
break because they proceed to the next command before the fake-key events have
taken effect.
Maybe it would have been better to just put the waits in the e2e tests in the
first case instead of in the production code? Well, we'll see. Maybe I'll
never have to deal with this again.
Exit prompt mode on the last test in downloads.feature because it's
preventing the next tests in editor.feature from running. Screenshots of
the first failing test in editor.feature shows a prompt open and
jsprompt open in the background.
This shouldn't happen as there is a module level fixture in conftest.py
that is supposed to close and re-open the browser process between each
feature file. That bears more examination but for now this change looks
pretty painless.
I've added a 10ms delay when sending a fake click via QEvents to help with a
timing issue in the prompt e2e tests in Qt6.7. Apparently that throws off the
tight timing of this one test! I guess the `:scroll bottom` comes before the
iframe has been clicked.
I'm a little worried that if we are depending on timing stuff like this we
should be looking at a more thorough solution. Maybe the "and follow" suffix
can trigger it to wait for "Clicked *editable element!"?
Also I can try to reduce the delay in the click, but 10ms is pretty short
already.
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.
A couple of tests seem to be failing because there is two windows open and
they don't expect it. I haven't fixed the root cause, I looked through the
logs and couldn't see why a second window was open. But if this makes the
tests pass, I guess we can address that if someone reports it.
Also changed `_get_scroll_values()` to handle multiple tabs/windows being open
too while I was there.
Example logs: https://github.com/qutebrowser/qutebrowser/actions/runs/8548730512/job/23422922448
Sessions file from the test logs:
Current session data:
windows:
- geometry: !!binary |
AdnQywADAAAAAAAyAAAAMgAAA1EAAAKJAAAAMgAAADIAAANRAAACiQAAAAAAAAAAAyAAAAAyAAAA
MgAAA1EAAAKJ
tabs:
- active: true
history:
- active: true
last_visited: '2024-04-04T03:21:00'
pinned: false
scroll-pos:
x: 0
y: 0
title: about:blank
url: about:blank
zoom: 1.0
- active: true
geometry: !!binary |
AdnQywADAAAAAAAyAAAAMgAAA1EAAAKJAAAAMgAAADIAAANRAAACiQAAAAAAAAAAAyAAAAAyAAAA
MgAAA1EAAAKJ
tabs:
- active: true
history:
- active: true
last_visited: '2024-04-04T03:21:03'
pinned: false
scroll-pos:
x: 0
y: 40
title: Scrolling
url: http://localhost:39235/data/scroll/simple.html
zoom: 1.0
Similarly to the last commit (982b8bdcec),
we run into a race between autofocus triggering and the test trying to
click the focused element.
This also affects the "when there is none" test, which loses the focus
before it has been set, thus ending up with a focused element but expecting
none.
We fix both in one go by manually focusing things with a tab keypress
instead of using autofocus.
See #8145 and #5390.
Fixes#8145, see #5390.
As long as we don't have a solution to get notified about focus happening
(#2471 possibly?), it looks like there is no better way to get notified
about this, so a delay will need to do for now.
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?
On CI now the sandbox test is failing on windows when we pop the header
line with an index error. It looks like the only line present on the
page is "Sandbox Status". It was working on CI the other day! Grrr
Hopefully it's a timing issue and the JS just hasn't finished running
yet? Not sure if just loading it again is the most reliable. Ideally we
would be listening for some event...
Pretty low effort but if this makes the test stop being flaky and we
don't have to look at it again that's fine with me.
In the linux branch when it was doing:
header, *lines, empty, result = text.split("\n")
assert not empty
It was complaining that "empty" was "}", because the windows sandbox
page has JSON at the bottom now. The whole things looks to have changed
completely. I'm actually surprised it was working before, why would it
have been saying seccomp was enabled on windows?
Anyway, I did the debug-dump-text --plain that quteproc is doing in a VM
and tested this with sandboxing off an on. No idea how stable that will
be!
ref: https://github.com/qutebrowser/qutebrowser/issues/7989
We replace the `(testdata)` placeholder with `testutils.abs_datapath()` in a few end2end
tests. So far we only needed to replace `(testdata)` with an OS path (e.g. in
`tests/end2end/features/spawn.feature`). By introducing `(testdata)` to an end2end test
in `tests/end2end/features/hints.feature`, we required a new use case: replacing
`(testdata)` as part of a valid file:// URI.
```
When I open file://(testdata)/some/file.txt
```
Replacing `(testdata)` in above BDD step with a plain OS path resulted in invalid URIs,
e.g. for the path "C:\\Users" above BDD step results in this invalid URI:
```
When I open file://C:\Users/some/file.txt
```
We deal with this by first isolating the `(testdata)` substitution in a single place.
Having `(testdata)` substitutions in a single place, we simply special-case the substitution
of file:// paths, which will be part of a URI.
Successful substitution for above BDD step looks like the following:
```
When I open file:///C:/Users/some/file.txt
```
As of Qt 6 navigating from local to remote origins requires user interaction. This broke
following links to remote origins via hints from a local file (e.g. `bookmarks.html`),
because hints use JavaScript to open links by default.
Example:
1. Have a local `bookmarks.html`:
```html
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>My bookmarks</title>
</head>
<body>
<a href="https://example.com/" id="link">some bookmark</a>
</body>
</html>
```
2. Open that local `bookmarks.html` in qutebrowser (e.g. `:open path/to/bookmarks.html`)
3. Start hinting
4. Follow a link to a bookmark (e.g. https://example.com/)
5. Instead of the link opening, be presented with `Your internet access is blocked`
error
To fix this, we simply force a user interaction for all hints on file:// URLs (like we
did for `qute://` URLs in 8defe1ae44).
We skip the end2end test for webkit, because webkit does not support URLSearchParams[1]
used in `tests/end2end/data/hints/link_inject.html`
[1] https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams
Given the following scenario:
```
When I open file://(testdata)/some/file.html
Then file://(testdata)/some/file.html should be loaded
```
If the end2end data directory is not normalized, the scenario fails because we try to
compare
```
file:///home/palbrecht/dev/qutebrowser/tests/helpers/../end2end/data/hints/link_inject.html?port=50…
```
to
```
file:///home/palbrecht/dev/qutebrowser/tests/end2end/data/hints/link_inject.html?port=50…
```
Normalizing the path resolves the `..` and fixes the issue.
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
```
With Qt 6.3+, user interaction is required to navigate outside of qute:// from a
qute:// page.
Follow-up to 8defe1ae44.
Also see 216a9f9a9bFixes#7815
See #7220 - should be revisited once we have a qute-bookmarks:// instead where
we can adjust permissions when registering the URL handler.
Opening a download in a new tab leaves a "dead" tab (see example of a "dead" tab below)
behind. When saving a session containing such a "dead" tab, we end up with entries in
the session like this one:
```yaml
- active: true
history:
- active: true
last_visited: '1970-01-01T02:00:00'
pinned: false
scroll-pos:
x: 0
y: 0
title: ''
url: ''
zoom: 1.0
```
When loading a session containing such a "dead" tab, qutebrowser does not restore any
history of that session and logs the following error:
```
ERROR: Failed to load session default: PyQt6.QtCore.QUrl('') is not valid
```
As pointed out by @The-Compiler in this comment[1], the behavior of
`QWebEngineHistoryItem::isValid()`[2] changed somehow between Qt 6.4 and 6.5.
`QWebEngineHistoryItem::isValid()` now returns `True` for "dead" tabs, even though the
history item is not valid (i.e. `url().isValid()` returns `False`).
To fix this we simply add an additional check if the URL is valid before adding a tab to
the session to be saved.
[1] https://github.com/qutebrowser/qutebrowser/issues/7696#issuecomment-1672854592
[2] https://github.com/qt/qtwebengine/blob/v6.5.2/src/core/api/qwebenginehistory.cpp#L69-L75
See 5ff0a573f4
With that commit, encoding the header ourselves means that we'll actually be
navigating to the path `/b'data/...'` instead of `/data/...`.
Group commands related to commands/commandline by prefixing them with `cmd-`. In this
case we additionally renamed the command slightly to fit better with the `cmd-` prefix.
Group commands related to commands/commandline by prefixing them with `cmd-`. In this
case we additionally renamed the command slightly to fit better with the `cmd-` prefix.
With Qt 6.3+, user interaction is required to navigate outside of qute:// from a
qute:// page.
Fixes#7815
See #7220 - should be revisited once we have a qute-bookmarks:// instead where
we can adjust permissions when registering the URL handler.
Needed mostly for urlmarks BDD tests so they can clear things between tests.
Hopefully with --all, this won't be accidentally triggered by users.
Preparation for #7815
While not documented that way:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/cwait?view=msvc-170
It looks like that Windows sometimes sets errno to EACCES here (causing a
PermissionError):
> os.waitpid(pid, 0) # pid, options... positional-only :(
E PermissionError: [Errno 13] Permission denied
I have no idea why it happens, but it results in flaky tests on CI.
We aren't particularly interested in this (we just want to make sure the process
is cleaned up before the next test runs...), so let's just ignore this.
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'
```
For unknown reasons, on nightly CI we now get TWO errors:
<qutebrowser.browser.webkit.certificateerror.CertificateErrorWrapper errors=['NoPeerCertificate', 'SelfSignedCertificate'] string='The peer did not present any certificate\nThe certificate is self-signed, and untrusted'>
TL;DR: I think I stabilized a couple of hinting iframe tests and applied
that change to all the iframe usages in the end2end tests in the hopes
that it would resolve some other flaky tests.
I was facing some tests that hinted elements in iframes failing
intermittently recently. They were most consistently failing on the
windows runner. This is similar to the trend described in the comment
linked from #1525.
The tests failing recently where:
tests (py39-pyqt515, windows-2019, 3.9)
test_using_hintfollow_inside_an_iframe
test_using_hintfollow_inside_an_iframe_button
tests (py311-pyqt515, ubuntu-20.04, 3.11)
test_using_hintfollow_inside_an_iframe
test_using_hintfollow_inside_an_iframe_button
They are failing because hints don't get generated for the elements in
the iframe. I can see hints do get generated for the iframe itself
though.
Examining the logs in 5.15 it seems that there is a
`cur_load_finished(True) (tab *)` log line after we run :hint. I suspect
that this is the load finished signal for the iframe.
I attempted to change the bdd open_path function to wait for the current
message it's looking for and then additionally look for that
cur_load_finished signal, but then it starts failing locally when the
iframe load signal actually comes before the parent frame one. Just
looking for the cur_load_finished signal itself also always found it
immediately ("already found"), probably from the parent frame.
So instead of trying to deal with that indeterminate ordering or
trying to change the signals to say what frame they are coming from, I
added javascript to all the pages used in iframes that run on load, and
changed all the tests to watch for log messages from the JS.
It's not the most maintainable solution, perhaps if we generated our
test files with jinja we could have some "subframe loaded" boilerplate,
get a message to log from a query param, look at metadata from test
files for the open_path checking etc.
I then searched for all the iframe usages in the test data and applied
that change to all of those files and all the tests that used them. A
few of those tests also had `flaky` annotations on them. I removed those
annotations for now in the hopes that there were affected by the same
problem. And that I actually managed to work around it correctly.
ref: #7621
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
There's some weird issue with Qt6.4 and 6.5 where a webengine view gets
its widget swapped out when it gets history deserialized into it. While
it's swapping widgets it has no so the focus gets passed to some other
widget should never even have focus.
There's probably more comprehensive ways we could handle this, by
overriding the WebEngineView layout, or by overriding focusNextPrevChild
on the parent to put out own logic into the "child is going, pass focus
to parent" logic chain.
But all that seems like a bit too much of a headache for this very
focussed issue. We want the new tab to get focus, so lets re-focus it.
Technically we could just focus the last tab that we open, if we are
undoing multiple, but the existing logic is to open each of them in turn
as foreground tabs and this reinforces that.
Closes: #7623
Not sure why its failing on windows, but it only really needs to run in
one environment anyway.
Previously this was using chrome://gpu but that was failing on CI due to
it invoking some vulkan stuff the didn't work without a GPU. I changed
it to chrome://sandbox/ for no good reason, but it seems to be working
fine on ubuntu at least.
Follow up to 151d808940
This one is for 6.5. Interesting that it was fine on 6.4.
I haven't gone and added text to all the test file because I'm hoping
it'll be something that upstream addresses in short order. So I'm only
adding text to files used by failing tests.
See d413b87c3f#7621 and #7624
On GH actions I'm seeing an "unexpected line" failure around creating a
vulkan context when loading chrome://gpu
ERROR tests/end2end/features/test_tabs_bdd.py::test_cloning_a_tab_with_a_special_url - end2end.fixtures.testprocess.InvalidLine:
1829
Error: vkCreateInstance failed with VK_ERROR_INCOMPATIBLE_DRIVER
1830
at CheckVkSuccessImpl (../../../3rdparty/chromium/third_party/dawn/src/dawn/native/vulkan/VulkanError.cpp:88)
1831
at CreateVkInstance (../../../3rdparty/chromium/third_party/dawn/src/dawn/native/vulkan/BackendVk.cpp:360)
1832
at Initialize (../../../3rdparty/chromium/third_party/dawn/src/dawn/native/vulkan/BackendVk.cpp:235)
1833
at Create (../../../3rdparty/chromium/third_party/dawn/src/dawn/native/vulkan/BackendVk.cpp:165)
1834
at operator() (../../../3rdparty/chromium/third_party/dawn/src/dawn/native/vulkan/BackendVk.cpp:420)
I'm not sure its actually failing the test, we are just seeing the error
in the logs and flagging it.
Instead of adding the logs to an ignore list I'm going to switch the
page we use so that if the error comes up for real we'll be sure to see
the logs.
I don't quite understand why the value changed from #00000 to #121212
there with Qt 6.3, but it looks like the change is here to stay.
Instead of keeping #000000 the default and adding an override for every
new Qt release, let's just switch over and add an override for the old
versions (5.15 and 6.2), so this won't break again for every new release.
See #7624
This reverts commit 47be6f3aeb.
This doesn't really make sense in test files, as the warning would point
inside some internal pytest code, which is not helpful.
Instead, let's just disable the corresponding flake8 warning in tests.
flake8-bugbear B028 No explicit stacklevel keyword argument found. The
warn method from the warnings module uses a stacklevel of 1 by default.
This will only show a stack trace for the line on which the warn method
is called. It is therefore recommended to use a stacklevel of 2 or
greater to provide more information to the user.
Semgrep helped:
semgrep --lang=py -e 'warnings.warn($ARG)' --replacement 'warnings.warn($ARG, stacklevel=2)' $FILES -a
semgrep --lang=py -e 'warnings.warn($ARG1, $ARG2)' --replacement 'warnings.warn($ARG1, $ARG2, stacklevel=2)' $FILES -a
Although it did lose the f-string on one of them.
Mostly pretty lazy fixes. Most of the places in the tests we were
already matching on error message, a couple of places we weren't. The
tick-tock one was the only one that wasn't being used right where it was
raised.
Some of them I just changed to RuntimeError because it was shorter than
adding the pylint directive.
The "Answering a question for a cancelled download" test was flaky,
due to the preceding PDF.js test sometimes not waiting properly for its
download to finish: "I wait until the download is finished" waited for
the initial PDF download above (which causes PDF.js to open), but not
for the real download after clicking the save button.
See #5390
TODO: Cherry-pick to master?
With Qt 6, the default changed to Qt handling network redirects instead of us
doing so manually. This seems like a good thing, so instead of setting the
redirect policy back to QNetworkRequest.RedirectPolicy.ManualRedirectPolicy,
let's just let Qt handle everything.
By default, Qt allows 50 redirects before giving up. That seems a tad much, so
we set it back to our former default.
This change comes with a few changes in behavior:
- Redirects to the same URL now fail (too many redirects) rather than being
ignored. I'm not sure how the previous behavior was useful. We added it in the
initial implemetation in 6856c49be9 (later
refactored a bit in 70e390a2e8) and added a test
in d13f88f0ac. But it doesn't make sense...
- We use QNetworkRequest.RedirectPolicy.NoLessSafeRedirectPolicy (no HTTPS ->
HTTP redirects allowed), while the former behavior didn't validate redirects
at all. Interestingly enough, I can't get Chromium to error out in that case
for downloads (though I only tried on localhost with a self-signed
certificate). However, it seems like a reasonable default.
Test will be added in a follow-up commit.
- Partially downloaded files aren't deleted anymore on a "too many redirect"
error. This should be solved in a more generic way, will do so in a follow-up
commit.
TODO: Pick to master?
Fixes#2679.
Not sure why we didn't do this initially, maybe it wasn't possible: https://webpy.org/cookbook/ssl
This means we don't need to (ab)use the Flask development server anymore.
We used to dodge these by listening on 0.0.0.0. Now they are on to us
and always show the warnings. Running flask in development mode here is
intended, so lets not have the warnings fail the tests.
Ref: https://github.com/pallets/werkzeug/issues/2480
toofar
Florian Bruhin
Evan Chen
Michael M
Michael M
Philipp Albrecht
Kian-Meng Ang