Skip to content

TST: allow to check for specific xlrd version and skip reader test if xlrd < 0.9 #5058

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

Merged
merged 1 commit into from
Oct 2, 2013

Conversation

yarikoptic
Copy link
Contributor

See http://nipy.bic.berkeley.edu/builders/pandas-py2.x-wheezy-sparc/builds/148/steps/shell_4/logs/stdio for failure when outdated xlrd found (e.g. on debian stable wheezy)

@jreback
Copy link
Contributor

jreback commented Oct 2, 2013

@cpcloud @jtratner seems reasaonable....

@jtratner
Copy link
Contributor

jtratner commented Oct 2, 2013

fine with me. At some point probably better to use LooseVersion, but seems like a good fix.

jtratner added a commit that referenced this pull request Oct 2, 2013
TST: allow to check for specific xlrd version and skip reader test if xlrd < 0.9
@jtratner jtratner merged commit fd3acfd into pandas-dev:master Oct 2, 2013
# If above test passes with outdated xlrd, next test
# does require fresh xlrd
# http://nipy.bic.berkeley.edu/builders/pandas-py2.x-wheezy-sparc/builds/148/steps/shell_4/logs/stdio
_skip_if_no_xlrd((0, 9))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have asked this before I merged, but what does this actually get you? Aren't you using the same exact version as before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bleh -- indeed! ;) sorry about that... here though it would specify version explicitly ;)

probably default version in _skip_if_no_xlrd could be downgraded somewhat. as that url from buildbot shows -- previous test passed fine with 0.6 version but next one failed with it. but that would need more testing... I might check tomorrow on that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't realize that's why you wanted to do that. You can't even find xlrd 0.6 on PyPI anymore (or at least I can't find it). But I guess that's what on Debian stable :-/. We'd have to discuss supporting xlrd 0.6 (because it'd also need to be a dep for one of the builds).

I would be -1 on different minimum versions for different parts of tests. Others? @jreback @cpcloud ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yarikoptic just an FYI you can compare versions much easier with the distutils.versions.LooseVersion class:

In [8]: from distutils.version import LooseVersion

In [9]: np.__version__ == LooseVersion('1.6.1')
Out[9]: True

In [10]: np.__version__ == LooseVersion('1.6')
Out[10]: False

In [11]: np.__version__ >= LooseVersion('1.6')
Out[11]: True

I'm also -1 on different versions of things for different tests, although i do something kind of like this for read_html (basically if bs4 version is 4.2.0 then i raise, but that's only tested in one place and i skip tests otherwise)

it would be ideal to have all of our tests support a single minimum version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, 02 Oct 2013, Jeff Tratner wrote:

Oh, I didn't realize that's why you wanted to do that. You can't even find
xlrd 0.6 on PyPI anymore (or at least I can't find it). But I guess that's
what on Debian stable :-/.
not only Debian stable:

$> whohas -D Debian,Ubuntu --strict python-xlrd | sort
Debian      python-xlrd                            0.6.1-2            119K            all                       http://packages.debian.org/squeeze/python-xlrd
Debian      python-xlrd                            0.6.1-2            119K            all                       http://packages.debian.org/wheezy/python-xlrd
Debian      python-xlrd                            0.9.2-1            140K            all                       http://packages.debian.org/jessie/python-xlrd
Debian      python-xlrd                            0.9.2-1            140K            all                       http://packages.debian.org/sid/python-xlrd
Ubuntu      python-xlrd                            0.6.1-2            118K            all                       http://packages.ubuntu.com/lucid/python-xlrd
Ubuntu      python-xlrd                            0.6.1-2            118K            all                       http://packages.ubuntu.com/precise/python-xlrd
Ubuntu      python-xlrd                            0.6.1-2            118K            all                       http://packages.ubuntu.com/quantal/python-xlrd
Ubuntu      python-xlrd                            0.6.1-2            118K            all                       http://packages.ubuntu.com/raring/python-xlrd
Ubuntu      python-xlrd                            0.9.2-1            140K            all                       http://packages.ubuntu.com/saucy/python-xlrd

Yaroslav O. Halchenko, Ph.D.
http://neuro.debian.net http://www.pymvpa.org http://www.fail2ban.org
Senior Research Associate, Psychological and Brain Sciences Dept.
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you must have monkey-patched Excel, right? ExcelFile mandates xlrd >= 0.9.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, 02 Oct 2013, Phillip Cloud wrote:

         with ExcelWriter(pth) as writer:
             self.frame.to_excel(writer, 'Data1')
             self.frame2.to_excel(writer, 'Data2')

  •        # If above test passes with outdated xlrd, next test
    
  •        # does require fresh xlrd
    
  •        # http://nipy.bic.berkeley.edu/builders/pandas-py2.x-wheezy-sparc/builds/148/steps/shell_4/logs/stdio
    
  •        _skip_if_no_xlrd((0, 9))
    

[1]@yarikoptic just an FYI you can compare versions much more easily with
the distutils.versions.LooseVersion class:

thanks -- I know it well ;) I just came up with the minimalistic patch
here. So +1 to replace that tuple comparison with LooseVersion(version)
comparison

I'm also -1 on different versions of things for different tests, although
i do something kind of like this for read_html (basically if bs4 version
is 4.2.0 then i raise, but that's only tested in one place and i skip
tests otherwise)

btw -- while at it... you might like how we 'centralized' version
checking in PyMVPA:

versions "dict" gets populated upon request for a given module:
https://github.com/PyMVPA/PyMVPA/blob/HEAD/mvpa2/base/externals.py
and then we have a generic helper (not per module e.g. _skip_if_no_xlrd):

skip_if_no_external('mdp', min_version='2.5')

which also could have max_version .

Cheers

it would be ideal to have all of our tests support a single minimum
version

well, imho it is ok sooner or later for some tests support new functionality of
external modules while older ones being as good for whatever uses only
old goodies

Yaroslav O. Halchenko, Ph.D.
http://neuro.debian.net http://www.pymvpa.org http://www.fail2ban.org
Senior Research Associate, Psychological and Brain Sciences Dept.
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to keep posting here, but the entire test suite fails if you try to use 0.6.1 (after monkey-patching to remove all the checks for xlrd). Please tell us what you mean here @yarikoptic .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, 02 Oct 2013, Jeff Tratner wrote:

In pandas/io/tests/test_excel.py:

@@ -350,7 +350,10 @@ def test_excelwriter_contextmanager(self):
with ExcelWriter(pth) as writer:
self.frame.to_excel(writer, 'Data1')

self.frame2.to_excel(writer, 'Data2')

  •        # If above test passes with outdated xlrd, next test
    
  •        # does require fresh xlrd
    
  •        # http://nipy.bic.berkeley.edu/builders/pandas-py2.x-wheezy-sparc/builds/148/steps/shell_4/logs/stdio
    
  •        _skip_if_no_xlrd((0, 9))
    

Sorry to keep posting here, but the entire test suite fails if you try to
use 0.6.1 (after monkey-patching to remove all the checks for xlrd).
Please tell us what you mean here [1]@yarikoptic .

I just wanted to address failure as in
http://nipy.bic.berkeley.edu/builders/pandas-py2.x-wheezy-sparc/builds/148/steps/shell_4/logs/stdio
which happened somehow here after preceding test... thus I assumed that
it might work ok with 0.6.1 ... may be it doesn't -- didn't check.

so whatever works for you -- works for me ;-)

Yaroslav O. Halchenko, Ph.D.
http://neuro.debian.net http://www.pymvpa.org http://www.fail2ban.org
Senior Research Associate, Psychological and Brain Sciences Dept.
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yarikoptic that's because the skip test is missing here :P let's change that instead.

@jtratner
Copy link
Contributor

jtratner commented Oct 3, 2013

minimum version I can get to pass the test suite is 0.8.

jtratner added a commit to jtratner/pandas that referenced this pull request Oct 3, 2013
Was mistakenly incorporated as a fix, when in reality it was a missing
skip test.

This reverts commit fd3acfd, reversing
changes made to ec54354.
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.

4 participants