biocore / pyqi

Tools for developing and testing command line interfaces in Python.
Other
9 stars 13 forks source link

Py33 #247

Closed wasade closed 10 years ago

wasade commented 10 years ago

still have those same two tests failing from the earlier pull request but this is much easier to look at on the change history

jairideout commented 10 years ago

Thanks @wasade! A few things:

wasade commented 10 years ago

Thanks for putting the list together. I should be able to get this knocked out today

On Thu, Jan 2, 2014 at 10:21 AM, Jai Ram Rideout notifications@github.comwrote:

Thanks @wasade https://github.com/wasade! A few things:

  • add mention in changelog that pyqi now natively supports python 2.7 and 3.3
  • update setup.py so that 2to3 isn't used, remove the assertion that python 2.7 is being used, and update the pypi classifiers string to reference python 3 (this way pyqi will be listed under pypi's python 3 packages)
  • update tox.ini so that python 3.3 is also tested against
  • update .travis.yml so that it uses the tox configuration to test against both python versions. See herehttp://borntyping.co.uk/posts/tox-and-travis-ci.htmlfor an example of how to have Travis work with tox, avoiding duplication between travis and tox configs
  • crawl through the pyqi docs and update places that reference python 2.7 to also include python 3.3

— Reply to this email directly or view it on GitHubhttps://github.com/bipy/pyqi/pull/247#issuecomment-31467883 .

wasade commented 10 years ago

done. still have the two failing tests though...

jairideout commented 10 years ago

Thanks @wasade! A couple of issues:

wasade commented 10 years ago

good catch on both, should be able to address on the bus

wasade commented 10 years ago

@jrrideout I'm not sure why travis isn't running against py3, its in the .yml file and in tox.ini

jairideout commented 10 years ago

@wasade the latest failure occurred because you're telling travis to run tox (which creates a virtualenv, installs, and tests pyqi) and then running the pyqi command after that, outside of the virtualenv. I think the pyqi command needs to be moved to tox.ini, and run after nosetests.

Travis isn't running against py33 because you're using tox to perform setup, installation, testing, etc. See the link I posted previously for instructions on how to get Travis and tox to play nicely together.

jairideout commented 10 years ago

Also note that if you made changes to tox.ini, they aren't showing up in this pull request.

wasade commented 10 years ago

ahh... ya, didn't add tox.ini, that would cause issues and clearly i wasn't looking close enough at the blog post. sorry about that WJ :)

i think this latest commit does it...

jairideout commented 10 years ago

For the second failure (pyqi.core.interface.get_command_config), we're returning str(e) as the error message, which differs between python 2 and 3.

Since we know the full module name that we're trying to import, one possible fix is to update get_command_config to report our own error message that references the unimportable module. This error message will be consistent across python versions. Thoughts?

jairideout commented 10 years ago

For the first failure (test_make_optparse.py), can you please post the full diff between observed vs. expected? This'll make it easier to determine how to proceed.

Jorge-C commented 10 years ago

The failure in test_make_optparse.py comes from the fact that in python 2, str(str) is "<type 'str'>" but in python 3, it's "<class 'str'>" and that's what gets compared in the test. type and class are synonyms so it's not really a problem. There's a lot of inheritance going on everywhere, and I'm not familiar with this code base, but I think the test could be invalid because it generates an OptparseOption instantiation with keyword argument Type=<type 'str'>. That is invalid syntax, and won't work as the input of InterfaceInputOption (OptparseOption ends up inheriting from it), because it would expect Type="str". So I think the solution is to replace DataType=str with DataType="str" and update win_text . Also, I guess the same thing would have to be done in the following line with DataType=bool.

jairideout commented 10 years ago

Thanks @Jorge-C! You're correct that make_optparse can create config files with invalid syntax. We haven't gotten around to fixing this yet, but it looks like now is a good time. :)

I think this can be fixed in pyqi.commands.make_optparse.MakeOptparse.run, line 156. Change it to:

data_type = cmdin.DataType.__name__

I tested this and it works in python 2 and 3. See this SO post for details.

wasade commented 10 years ago

Oh... ya, good call. I'd like to get this pull req done soon so hopefully tonight

On Tue, Jan 7, 2014 at 6:50 AM, Jai Ram Rideout notifications@github.comwrote:

Thanks @Jorge-C https://github.com/Jorge-C! You're correct that make_optparse can create config files with invalid syntax. We haven't gotten around to fixing this yet, but it looks like now is a good time. :)

I think this can be fixed in pyqi.commands.make_optparse.MakeOptparse.run, line 156https://github.com/bipy/pyqi/blob/master/pyqi/commands/make_optparse.py#L156. Change it to:

data_type = cmdin.DataType.name

I tested this and it works in python 2 and 3. See this SO posthttp://stackoverflow.com/q/5008828for details.

— Reply to this email directly or view it on GitHubhttps://github.com/bipy/pyqi/pull/247#issuecomment-31738202 .

wasade commented 10 years ago

@ElBrogrammer, I think this is good to go

wasade commented 10 years ago

One other thing, we should get travis to execute the env from python3 as well

jairideout commented 10 years ago

It looks like Travis is executing the tests with python 2.7 and 3.3. Take a look at the build matrix:

https://travis-ci.org/biocore/pyqi/builds/21111111

There are two links, one for each Python version.

wasade commented 10 years ago

nope, if you look at the output, it is only executing py27. the build output is using python27 and i suspect its the travis config. also, master right now fails on py33 until the py33 PR is merged

On Wed, Mar 19, 2014 at 11:15 AM, Jai Ram Rideout notifications@github.comwrote:

It looks like Travis is executing the tests with python 2.7 and 3.3. Take a look at the build matrix:

https://travis-ci.org/biocore/pyqi/builds/21111111

There are two links, one for each Python version.

Reply to this email directly or view it on GitHubhttps://github.com/biocore/pyqi/pull/247#issuecomment-38080042 .

jairideout commented 10 years ago

I think the output indicates it is executing py33 (see second link in the build matrix):

GLOB sdist-make: /home/travis/build/biocore/pyqi/setup.py
py33 create: /home/travis/build/biocore/pyqi/.tox/py33
py33 installdeps: nose
py33 inst: /home/travis/build/biocore/pyqi/.tox/dist/pyqi-0.3.1-dev.zip
py33 runtests: PYTHONHASHSEED='2543663536'
py33 runtests: commands[0] | nosetests

Travis is testing against its modified .travis.yml (the one in this PR), not the one in master.

wasade commented 10 years ago

THis is what I'm seeing in the travis build output, note the version of python shown at the bottom

$ export TOXENV=py33
git.1
$ git clone --depth=50 git://github.com/biocore/pyqi.git biocore/pyqi
Cloning into 'biocore/pyqi'...
remote: Counting objects: 1636, done.
remote: Compressing objects: 100% (934/934), done.
remote: Total 1636 (delta 762), reused 1502 (delta 657)
Receiving objects: 100% (1636/1636), 377.90 KiB | 0 bytes/s, done.
Resolving deltas: 100% (762/762), done.
Checking connectivity... done.
$ cd biocore/pyqi
git.3
$ git fetch origin +refs/pull/247/merge: 
remote: Counting objects: 436, done.
remote: Compressing objects: 100% (197/197), done.
remote: Total 307 (delta 175), reused 231 (delta 106)
Receiving objects: 100% (307/307), 40.65 KiB | 0 bytes/s, done.
Resolving deltas: 100% (175/175), completed with 36 local objects.
From git://github.com/biocore/pyqi
 * branch            refs/pull/247/merge -> FETCH_HEAD
git.4
$ git checkout -qf FETCH_HEAD
$ source ~/virtualenv/python2.7/bin/activate
$ python --version
Python 2.7.3
...snip...
jairideout commented 10 years ago

That's the Python version that Travis uses to set up a virtualenv before running the tests, which in this case is simply to run the tox command. I think tox then tests against py33 by creating its own virtualenv.

So that we're sure, can you please add a print statement or something so we can see what is actually happening?

wasade commented 10 years ago

running test now

jairideout commented 10 years ago

The print statement you added won't show up by default because nosetests suppresses stdout. However, I think we've proved that py33 is being run correctly since the py27 build passed and py33 didn't, due to print "foo" not working in py3. @wasade do you agree?

wasade commented 10 years ago

Agree, thanks for entertaining me on this. I'll remove the print in a sec

jairideout commented 10 years ago

No problem, and thanks for following up on this PR- will be exciting to get in!

wasade commented 10 years ago

@ElBrogrammer thanks for catching the pyqi driver in tox, there were a few remaining bits to address there

jairideout commented 10 years ago

Tests passed, let's bro out!!!

wasade commented 10 years ago