| Age | Commit message (Collapse) | Author |
|---|
|
This reverts commit ccebbd6ebbb787107523d71e51eb36bcdfdde759.
|
|
- 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 problem was that 'qute-input focused' wasn't logged. Let's see if this helps.
|
|
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.
|
|
Windows seems to struggle with the dash
|
|
|
|
- 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.
|
|
|
|
Dynamic dark mode
|
|
|
|
|
|
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 24d01ad25729458f0584a35c6b4d9a36f0b5e580, 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.
|
|
With pytest 8.2, pytest.importorskip(...) now only considers ModuleNotFoundError
rather than all ImportErrors, and warns otherwise:
https://github.com/pytest-dev/pytest/pull/12220
While we could override this via
pytest.importorskip(..., exc_type=machinery.Unavailable)
this is a simpler solution, and it also makes more sense semantically:
We only raise Unavailable when an import is being done that would otherwise
result in a ModuleNotFoundError anyways (e.g. trying to import QtWebKit on Qt
6).
|
|
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 (a209c86c55a4) 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 9f050c7460c42f317ceaa20b320e97d371a2c0a0.
Revert "Wait for evidence of Tab key press in tests before proceeding"
This reverts commit d47d247941c4b1fe3adac0dae8be301b36aec85b.
|
|
In the previous commit 9f050c7460c4 "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.
|
|
qutebrowser/feat/7187_chromium_security_patch_in_version
Show chromium security patch version in :version
|
|
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.
|
|
flake8 got a new warning about a module name shadowing a builtin module: https://github.com/gforcada/flake8-builtins/pull/121
Probably would have been safe enough to ignore it. But I don't think
moving it is that hard anyway. Hopefully I didn't miss anything!
|
|
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 (982b8bdcecfba6fc1687a6a4942b9e3ab3221eb7),
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.
|
|
|
|
Seems to break in bleeding edge tests
|
|
|
|
|
|
With a Qt 6.7 developer build, the tests fail with:
ASSERT failure in QtGlobalStatic::ApplicationHolder<QAS>::PlainType*
QtGlobalStatic::ApplicationHolder<QAS>::pointer() [with QAS =
{anonymous}::Q_QAS_qtlsbLoader; PlainType = QFactoryLoader]: "The
application static was used without a QCoreApplication instance", file
.../qtbase/src/corelib/kernel/qapplicationstatic.h, line 54
Fatal Python error: Aborted
[...]
Current thread 0x00007c18bb3f3740 (most recent call first):
File ".../tests/unit/browser/webkit/test_certificateerror.py", line 23 in <module>
See https://codereview.qt-project.org/c/qt/qtbase/+/495239
|
|
Webengine added a getter for their chromium patch level back in Qt 6.3,
since they backport security fixes from chromium in the periods between
doing major chromium feature upgrades.
It's pulled from a hardcoded string in the webengine source
`src/core/web_engine_context.cpp` that's manually updated when they
backport something.
The "(plus any distribution patches)" bit in there is because it was
pointed out that some distributions backport their own security patches
or even use webengine from a branch when the hardcoded string only gets
updated at release time, despite patches being backported in the
meantime.
Closes: https://github.com/qutebrowser/qutebrowser/issues/7187
|
|
It gives an AttributeError for both signal.SIGHUP and
signal.Signals.SIGHUP. The Signals Enum is set up so you can use the
strings to key into it though, so that's nice. I was tempted to use a
walrus operator here but I think that's python 310 and I don't remember
what our minimum supported version is.
|
|
It doesn't look like this class has any unit tests currently. And since
we are adding a new signal handler to it I took the opportunity to add a
few tests to it to establish a bit of a framework so that next time we
touch it it will be easier to add more.
I didn't go for full coverage here. It's an improvement over what was
there previously and it should make merging more palatable. I don't
think handling SIGHUP is a very risky feature.
I chose to use mocker.patch over monkeypatch.setattr because I think it
provides a more friendly and powerful API due to being more tightly
integrated with magic mocks. And with `mocker.patch.object` you can even
use actual object names instead of strings, the same as monkeypatch
allows.
One thing I'm less confident with here is mocking all multiple things in
a couple of fixtures. Writing one fixture for each little thing I mock
doesn't feel appealing to me right now, but for mocks that tests need to
access there isn't really any idiomatic way to let the tests access
them. I previously just had the one fixture but pulled out the read
config one for that purpose. It's a pattern I'll have to think on a bit
I think. Probably having to have developers thing about the balance of
boilerplate vs accessibility is cognitive load that we want to avoid.
Hmm.
Anyway, here are the options I was looking at to let test code access
mocks that where all shoved away into the one fixture:
1. group the tests into a class and put the mocks in class variables:
could work fine but I don't think it's very idiomatic for pytest?
2. return some kind of meta object from the fixture that has the object
under test as one attribute and the mocks as other ones: hmm,
returning multiple values from a method always seemed a bit smelly to
me
3. make one fixture for each of the mocks, have the main fixture depend
on them all, tests that need to modify them can depend on them too:
writing all those fixtures seems tedious and I don't think it would
be a clear win in terms of readability. *sigh*, I suppose I should
pull the ones I'm modifying out at least so other people don't copy
my lazyness in the future
4. have the test code access the mocks from the real code, eg
`configfiles.sys.exit.side_effect = ...`: this works fine but I feel
like it's relying on an implementation detail and probably shouldn't
be encouraged? Not sure
In case anyone is struggling with git blame and wondering what the
QSocketNotifier stuff is about, this commit should explain it: https://github.com/qutebrowser/qutebrowser/commit/210ce8ca7cb16e1d0ce859baa64ce56a5e1894cf
|
|
With the change in #7955, if the regex did not match, it ended up retrying the
lookahead at every position of the string (expected), but then *also* repeated
that process for every position in the string. Thus, a non-matching pattern
ended up in O(n^2) performance (with n = length of URL).
Instead, anchor the pattern at the beginning of the string. This doesn't change
behaviour as we use .* at the beginning of every lookahead anyways, but it means
we end up with O(n) instead of O(n^2) performance.
Co-authored-by: toofar <toofar@spalge.com>
|
|
This reverts commit 45d483479aebb6af5a53aea97d65228d8621ad86.
|
|
This reverts commit fe1faa14b91253db99e9b4451ffb1a479feff1db, reversing
changes made to 04af4c657d3725fd2d3b556d9b88b44d3ada0be1.
For my setup with 310 quickmarks and 94 bookmarks, this makes qutebrowser hang
for around a minute on every keypress.
|
|
https://docs.pytest.org/en/7.4.x/deprecations.html#py-path-local-arguments-for-hooks-replaced-with-pathlib-path
https://docs.pytest.org/en/7.4.x/reference/reference.html#std-hook-pytest_ignore_collect
8.0.0 removes the "path" compatibility alias we used.
|
|
As shown in detail by toofar in dff25d10bcc8a398a77ef260c33e51ec5c70f482,
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?
|
|
A few more completions will now match search terms in any order:
`:quickmark-*`, `:bookmark-*`, `:tab-take` and `:tab-select` (for the quick
and bookmark categories).
|
|
See #7837
|
