Skip to content

Conversation

@The-Compiler
Copy link
Member

See #221

Last one for now 😉

@coveralls
Copy link

coveralls commented Sep 13, 2018

Coverage Status

Coverage decreased (-1.05%) to 98.049% when pulling 8009c54 on The-Compiler:modeltester-update into 0a19255 on pytest-dev:master.

@The-Compiler
Copy link
Member Author

I think I'll need some help with Qt 4 here. Looking at it, I don't think the type checks ever worked properly there, but our tests only tested wrong types and the DisplayRole, which happened to be added to the checked roles with the upstream Qt changes.

I'm not really sure what the right way to make this work is. I think before 41b83ac the checks didn't make much sense at all, and c2403a0 seems to be needed for Python 2. But now I also see QString in the tests, so apparently the allowed types should be one of:

  • str (Python 3)
  • unicode (Python 2)
  • QString (if available)

or something?

@nicoddemus
Copy link
Member

You are right that it seems previously nothing was tested properly.

PyQt4 originally (the pyqt4 API) always returned native QString objects. The V2 API (pyqt4v2), introduced later, returns native Python types instead. So in Python 2 I assume those are always str objects in some who-knows-what encoding. At work we do something like qstring.utf8().decode('utf-8') to obtain a unicode object.

Well it is quite a pickle. I pushed some tweaks to that in de8702b, let's see how it fares.

@The-Compiler
Copy link
Member Author

Oh, seems to look better now, thanks! Not sure if I should also add some more tests, but on the other hand I think the actual test coverage (i.e. "cases" of broken models we test) didn't really change much.

@nicoddemus
Copy link
Member

I guess we can add the changelog entry later before the release. 👍

@nicoddemus nicoddemus merged commit 9c233d7 into pytest-dev:master Sep 18, 2018
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