Initial migration was just:
import yaml
with open('.eslintrc.yaml') as f:
config=yaml.load(f.read(), yaml.CLoader)
with open("eslint.config.js", "w") as f:
json.dump(config, f)
Then add a new ignores key, figuring out how to import the default "all"
config, and the globals.
Added some ignore lines for undefined globals in the config file since
they are probably legitimately not available in a browser.
The `ignores` config item doesn't seem to be working, not sure why.
Other than that theres not many warnings from the files that are
actually supposed to be linted, so that's good!
I have no-idea how much of the rest of the config is or isn't working.
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
In the bleeding edge CI tests I'm seeing:
IGNORED: Path override failed for key base::DIR_APP_DICTIONARIES and path '/__w/qutebrowser/qutebrowser/.tox/bleeding/lib/python3.11/sitePath override failed for key base::DIR_APP_DICTIONARIES and path '/__w/qutebrowser/qutebrowser/.tox/bleeding/lib/python3.11/site-packages/PyQt6/Qt6/libexec/qtwebengine_dictionaries'
INVALID: -packages/PyQt6/Qt6/libexec/qtwebengine_dictionaries'
Where the second line looks like maybe spill over from the first one
that is for some reason being printed twice and interleaved! Maybe
different processes or threads?
I did a regex for "line ending with this and with no spaces in it" which
is hopefully safe enough to not pick up a wrong line accidentally.
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.
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
jaraco added some utility libraries they maintain to the keyring package
that they also maintain.
Also update changelog URLs to be consistent since they have a "skeleton"
project which lays everything out and publishes all their projects the
same. Should we be preferring GH or RTD links? idk
The lint ones are:
linters (pylint): qutebrowser/completion/completionwidget.py#L440
Consider using 'height = min(height, contents_height)' instead of unnecessary if block
linters (pylint): qutebrowser/browser/webengine/webview.py#L241
Useless suppression of 'no-member'
The no-member one might be due to this change: https://github.com/pylint-dev/pylint/issues/9246
For the importlib-resources one, I'm not sure why it's changed to be
underscore instead of a dash now. But at least it's consistent across
all the requirements files now!
I feel like I've seem this in a previous dependancy update too (for a
different package?) but I can't find that now.
Currently the unstable docker images are failing to build
(undefined symbol: _ZN5QFont11tagToStringEj, version
Qt_6. Looks like Qt has upgraded to 6.7 but pyqt6 hasn't been patched to
remove some symbols that are gone now).
But we might as well let the stable ones rebuild right?
It seems `sed -i` is not very portable. Initially we were using this
command:
sed -i '' '/.-d., .--debug.,/s/$/ default=True,/' qutebrowser/qutebrowser.py
and then that started breaking on windows, I'm not sure why, with "can't
read /.-d., .--debug.,/s/$/ default=True,/: No such file or directory".
Then we changed to:
sed -i '/.-d., .--debug.,/s/$/ default=True,/' qutebrowser/qutebrowser.py
so without the extension argument, but that broke on mac with "1:
"qutebrowser/qutebrowser.py": extra characters at the end of q command"
then we tried:
sed -i'' '/.-d., .--debug.,/s/$/ default=True,/' qutebrowser/qutebrowser.py
and that also broke on mac with the same error. On the recommendation of
stackoverflow I just changed it no not use in-place editing and do a
good old fashioned move afterwards. https://unix.stackexchange.com/questions/92895/how-can-i-achieve-portability-with-sed-i-in-place-editing
... record scratch ...
Apparently these GHA steps are being run in powershell in windows where
`mv` is implemented by `Move-Item` where you have to use -Force to
overwrite destination files.
But that's not portable. cp does happily overwrite without any
additional instruction though. So I'm doing cp instead of mv and then
removing the temp file.
Probably if this drags out anymore we should download something off of
pypi which is platform independent to handle it.
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: 210ce8ca7c
Before this commit qutebrowser would just exit when receiving a SIGHUP.
Now it will reload the config using the bare config_source function, just like
the :config-source command does.
Closes#8108.
toofar
Florian Bruhin
qutebrowser bot
dependabot[bot]
tarneo