-
Notifications
You must be signed in to change notification settings - Fork 7
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
Introduce Pytest, Tox, Coverage #19
Conversation
from setuptools import setup, find_packages | ||
from minecraft import meta | ||
|
||
readme = io.open('README.rst', mode='r', encoding='utf8').read() |
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 can use with open()
.
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.
The default open
is not a good idea because it does not always handle encoding correctly, see strongswan/swidGenerator#35. If the system has a terminal which is not set to UTF-8, it could cause problems.
Using a context manager is a good idea though.
@dbrgn – This is a good start but I don't think we need to target 3.4 as Raspbian doesn't ship it yet. Thanks for making the change to Tox! |
That said, I think we still want to work with 3.4 (for when Raspbian does ship it), but we need it to work for 3.2 (Wheezy stable) |
Exactly, 3.2 should be the main goal right now, but eventually the library will be used under Python 3.4. I'd suggest we keep the versions 3.3 and 3.4 in tox, but remove them from Travis, what do you think? (Or we could flag them as allowed failures.) |
@dbrgn @doismellburning – No I think those are fair points, lets keep both versions in ready for when they're available. |
- Tests are run using pytest - Test environments are set up using tox - Right now, Travis-CI is set up using a Qemu installation, in which tox is run - Coverage is measured - Coding guidelines are enforced
OK, feedback has been integrated and the branch has been rebased. Right now there's a build matrix on Travis with Python 2.7 and 3.2. There's also a build that does coverage testing, with an allowed failure. In there we could add a tool like Coveralls. The tests pass on Python 2.7 and fail on all Python 3 versions. In the "cov" build you can see an ASCII coverage report: |
That Py3 failure looks...fun :s |
Yay for working CI builds! OTOH, it's a bit disappointing that it fails on As for the QEMU thing: that was me following instructions from a random On Tue, Sep 30, 2014 at 10:26 AM, Danilo Bargen [email protected]
|
Yeah, it's slow and tedious but I definitely like the idea of Actual Realistic Testing - not sure I have any bright ideas about good ways to speed it though... (DIY Jenkins testing against pre-built emulator images? That way at least we're pre-loading the work from most of https://github.com/py3minepi/py3minepi/blob/master/.travis-ci.sh, otoh that's a tedious maintenance requirement - I think I'd rather just have slower tests...) |
Anyway this looks good. Can I confirm that this is a net improvement, i.e. some tests now pass whereas none did previously? |
+1 on it being a net improvement, and +1 for the overall idea of the change. |
I'm in agreement with @pozorvlak, it's a net improvement and adds some of the items we discussed in #14. |
Probably some issue with identity comparison. But that test doesn't really matter anyways. Most of the code is still untested.
Yes, mocking. But mocking is always based on assumptions. We can just test what gets sent to Minecraft, not what Minecraft does with it.
While that sounds good, how do we actually write the assertions? There's no way to test whether a set_block command actually created a block. And Minecraft does not return any output on errors. So basically there's no way to test any assertions using the Minecraft binary. |
Why can't we send On Tue, Sep 30, 2014 at 12:29 PM, Danilo Bargen [email protected]
|
Oh, true, we can do that... Sorry for forgetting :) It's an integration though, not a unit test. For unit tests, we should use mocking. It's faster anyways. In that case I'll merge this. |
Please never again merge your own pull request, thanks |
@doismellburning I thought the three confirming comments above were a "good to merge". But in that case I'll leave it to others in the future :) |
@dbrgn: absolutely, anything that touches the Minecraft binary is an On Tue, Sep 30, 2014 at 2:20 PM, Danilo Bargen [email protected]
|
Fixes #13.
Still work in progress. Qemu on Travis is very slow :/
The question is whether we actually need Qemu with a real server, or if we can go with mocking one. Then we could get rid of the Qemu setup.