Skip to content

Conversation

@The-Compiler
Copy link
Member

This makes it possible to pass custom arguments to Qt.

  • I'm not sure how to test this yet - I can try something with QCoreApplication.arguments() and a subprocess, but I'm not sure whether it works.
  • I'm also not sure where to document it, given that qapp isn't documented at all either. What do you think?

This makes it possible to pass custom arguments to Qt.
The-Compiler added a commit to qutebrowser/qutebrowser that referenced this pull request Nov 20, 2017
This is a stop-gap so I'm able to run end2end tests at least.
See #3163.

For unit tests, we need pytest-dev/pytest-qt#193 first.
@coveralls
Copy link

coveralls commented Nov 20, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 391661f on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

@nicoddemus
Copy link
Member

Hey @The-Compiler!

I can try something with QCoreApplication.arguments() and a subprocess, but I'm not sure whether it works.

I think this is the way to go. Perhaps printing QCoreApplication.arguments() and watching the output would be enough?

I'm also not sure where to document it, given that qapp isn't documented at all either. What do you think?

I think adding a reference to https://github.com/pytest-dev/pytest-qt/blob/master/docs/reference.rst is enough (for both qapp and qapp_args).

@The-Compiler The-Compiler changed the title WIP: Add a qapp_args fixture Add a qapp_args fixture Nov 20, 2017
@The-Compiler
Copy link
Member Author

Hmmm - things seem to work fine on Travis so far (and for me locally), but fail on AppVeyor:

>       assert qapp.arguments() == ['--test-arg']
E       AssertionError: assert <PyQt4.QtCore...0000003F97DD8> == ['--test-arg']

Unfortunately the ellipsis don't really help here 😆. Do you know what's going on there? Do we get a QStringList or something?

@coveralls
Copy link

coveralls commented Nov 20, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 43c2d1c on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

@The-Compiler
Copy link
Member Author

Oh, and it looks like there are some other environments also include the arguments of pytest there, but only on Windows? I'll try writing a more forgiving test and see how that goes.

@coveralls
Copy link

coveralls commented Nov 20, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 2b997c1 on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 2b997c1 on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

@nicoddemus
Copy link
Member

Unfortunately the ellipsis don't really help here 😆. Do you know what's going on there? Do we get a QStringList or something?

Weird. Well I changed the code to explicitly making a list and call str() on each argument, let's see how that goes.

@coveralls
Copy link

coveralls commented Nov 20, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 7514958 on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 7514958 on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

@The-Compiler
Copy link
Member Author

I think there are two different issues here:

  • The arguments are a string list in some scenarios. I can actually reproduce this when trying with PyQt by hand, and it looks like doing list() without calling str() on the arguments suffices:
>>> from PyQt4.QtGui import QApplication
>>> app = QApplication(['--test-arg'])
>>> app.arguments()
<PyQt4.QtCore.QStringList object at 0x7f6dde415500>
>>> list(app.arguments())
[PyQt4.QtCore.QString(u'--test-arg')]
>>> '--test-arg' in list(app.arguments())
True

So I guess we can back out your last commit 😉

  • In some environments, we get the sys.argv passed to pytest instead of the arguments passed to Qt. This seems like a Windows + Qt4 thing? I wonder whether we should just skip the test there. I wonder how PySide2/PyQt5 look, but I'm off to bed now.

@nicoddemus
Copy link
Member

nicoddemus commented Nov 20, 2017

I wonder whether we should just skip the test there

Sounds reasonable I guess. Let's continue this discussion tomorrow then. 👍

@coveralls
Copy link

coveralls commented Nov 21, 2017

Coverage Status

Coverage decreased (-2.9%) to 97.059% when pulling 80cbdfd on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

@coveralls
Copy link

coveralls commented Nov 21, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 086b95d on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 086b95d on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 97ec466 on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 97ec466 on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

@coveralls
Copy link

coveralls commented Nov 21, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.328% when pulling 97ec466 on The-Compiler:qapp-args into 1d33878 on pytest-dev:master.

@The-Compiler
Copy link
Member Author

Phew - after needing three tries to get a working version checks, things look fine now 😆

The PySide2 errors are unrelated:

=================== 277 passed, 34 skipped in 28.79 seconds ====================
Fatal Python error: deallocating None
Current thread 0x00007fb949183740 (most recent call first):
Aborted (core dumped)

Working on a project which supports 3.5 Qt wrappers with 2 major Python versions on 2+ operating systems sure isn't easy sometimes 😆

This is ready now as far as I'm concerned.

@nicoddemus
Copy link
Member

Thanks @The-Compiler!

Working on a project which supports 3.5 Qt wrappers with 2 major Python versions on 2+ operating systems sure isn't easy sometimes

Indeed! 😆

This is ready now as far as I'm concerned.

Yep! I will take a closer look at the errors when I get home, but either way we can probably merge this and push a release. Would you like to do the honors? All is needed is to push a tag and Travis will publish the package.

@The-Compiler
Copy link
Member Author

Sure, let's see how that goes! 🎉

@The-Compiler The-Compiler merged commit 4dfb65f into pytest-dev:master Nov 21, 2017
@The-Compiler The-Compiler deleted the qapp-args branch November 21, 2017 15:34
@The-Compiler
Copy link
Member Author

Looks like that worked! Do you usually write an announce mail or something?

@nicoddemus
Copy link
Member

I usually mention it on twitter, adding a link to the changelog. Want to do the honours here as well? 😉

@The-Compiler
Copy link
Member Author

The-Compiler commented Nov 21, 2017

Hah, looks like you were faster! 😉

@nicoddemus
Copy link
Member

I thought you were asleep by now so decided to send the tweet myself. 😝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants