-
-
Notifications
You must be signed in to change notification settings - Fork 70
Add qtbot.waitCallback, take 2 #236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
nicoddemus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments @The-Compiler, truly standing work!
|
|
||
| qt_api.set_qt_api(config.getini("qt_api")) | ||
|
|
||
| if config.getini("qt_wait_signal_raising"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we check if config.getini("qt_wait_signal_raising") returns True or False to raise the warning? Because if users configure it as False, the if block won't execute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think it's a string (because there's _parse_ini_boolean in qtbot.py), but I wonder why we don't pass type="bool" in the parser.addini calls in pytest_addoption to let pytest do the ini parsing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you see the warning if qt_wait_signal_raising is set to False or True, I'm happy!
|
Oh also please merge/rebase with |
| res = testdir.runpytest() | ||
|
|
||
| if configkey == "qt_wait_signal_raising" and configval is not None: | ||
| with pytest.warns(DeprecationWarning): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already should make sure the warning is shown with qt_wait_signal_raising = false in the ini as well, as there's a parametrization with configval = "false".
|
We also should add a new changelog entry. |
|
Rebased (with some trivial fixes squashed) and added a changelog! |
filterwarnings was not a registered marker until 3.7.2
|
Had to push another commit requiring pytest>=3.8... for some reason a few jobs were using an old pytest version. We need 3.7.2 at least because filterwarnings was not a registered mark until then. |
|
Oooh we have a bigger problem then. That's the reason why it picked a pytest 3.2.3, because it was the last version before we started to depend on We need to drop Python 3.4 support or the strict setting from |
|
Can't we just register it ourselves in Given that |
This reverts commit 2a5277b.
|
FWIW https://pypi.org/project/atomicwrites/ seems to claim supporting 3.4 just fine, and I also don't remember running into any issues - seems like a conda packaging bug or something? |
|
Conda forge does not have that package for 3.4 it seems, only 3.5 onward. Please go ahead with the setup.cfg solution, didn't know it was possible. 👍 |
|
Want to do the honors of doing the next release by the way? |
|
Sure, why not! IIRC it was all automated after pushing a tag, right? Probably after #238 though, will also update the changelog there once this is in. |
|
Yep, it is very simple, there's the HOWTORELEASE doc in the repo with the instructions 👍 |
|
AppVeyor failed with: Doesn't look like I can trigger a rerun myself. |
|
Oh sorry just now saw this. I've triggered a new build. Unfortunately it seems pytest-qt is under my personal account. But I added you anyway as user, when you have the time see if you have permission to cancel the appveyor build, or trigger a re-build. |
|
Fixed the conflict! 👍 |
|
Seems to look good now! :) |
|
|
||
| if raising is None: | ||
| raising = self._request.config.getini("qt_wait_signal_raising") | ||
| raising = self._should_raise(raising) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that I actually fixed another bug here 😆 qtbot.waitSignals() didn't call _parse_ini_boolean like in other places, so raising was "" (so false) when there is no entry for it in the .ini file. This actually ended up hiding a failing test in qutebrowser!
Based on #157, but up-to-date, and with docs and tests and all.