Skip to content

BUG: off-by-one when doing network test repeats #4485

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

BUG: off-by-one when doing network test repeats #4485

wants to merge 2 commits into from

Conversation

esc
Copy link
Contributor

@esc esc commented Aug 6, 2013

Imagine num_runs is two, then runs will take on the values [0, 1] and runs <
num_uns will always be False. Hence any genuine Exception the code might
raise will be silently ignored and just printed.

@cpcloud
Copy link
Member

cpcloud commented Aug 6, 2013

thanks for catching this, can you hook up travis ci

cpcloud
cpcloud reviewed Aug 6, 2013
View reviewed changes
pandas/util/testing.py
@@ -816,7 +816,7 @@ def network_wrapper(*args, **kwargs):
except SkipTest:
raise
except Exception as e:
if runs < num_runs:
if runs < num_runs - 1:
Copy link
Member

Choose a reason for hiding this comment

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

i think if runs <= num_runs: is clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how that would help, or did I miss something?

In [6]: 0 < 2
Out[6]: True

In [7]: 1 < 2
Out[7]: True

In [8]: 0 <= 2
Out[8]: True

In [9]: 1 <= 2
Out[9]: True

In [10]: 0 < 2 -1
Out[10]: True

In [11]: 1 < 2 -1
Out[11]: False

Copy link
Member

Choose a reason for hiding this comment

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

whoops, sorry, you're right...don't know what i was thinking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, two hard problems in computer science, cache invalidation and naming things and off-by-one errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

@esc haha - very clever :)

@esc
Copy link
Contributor Author

esc commented Aug 6, 2013

Not sure what to do about the Travis error though.

@esc
Copy link
Contributor Author

esc commented Aug 6, 2013

Think it has been failing all along, but unnoticed.

@jreback
Copy link
Contributor

jreback commented Aug 6, 2013

this is marked as issue #4427 (the failing test), which was reported.....

@esc
Copy link
Contributor Author

esc commented Aug 6, 2013

My guess is, it should look like 46f6c43

@esc
Copy link
Contributor Author

esc commented Aug 6, 2013

I see, well in that case, let's just ignore the fix I made?

@jreback
Copy link
Contributor

jreback commented Aug 6, 2013

@esc your fix is good; was just linking the issue that needs to be fixed (your are fixing the test that was not catching this issue) thxs!

@esc
Copy link
Contributor Author

esc commented Aug 6, 2013

Doesn't fail on master though 😕

@jreback
Copy link
Contributor

jreback commented Aug 6, 2013

That test needs prob just needs to come out...it really just needs to check that the reader 'works', rather than comparing a value which gets revised (and of course they don't tell you that!)

@jtratner
Copy link
Contributor

jtratner commented Aug 7, 2013

I'm +>9000 on removing that test!

@jreback
Copy link
Contributor

jreback commented Aug 7, 2013

@esc why don't u put a skip test in the failing test that you uncovered

@jtratner
Copy link
Contributor

jtratner commented Aug 7, 2013

btw - the original PR is a nice object lesson for why tests are important for correct code 😅

@esc
Copy link
Contributor Author

esc commented Aug 7, 2013

And now I get this one:

https://travis-ci.org/esc/pandas/jobs/9918210

@esc
Copy link
Contributor Author

esc commented Aug 7, 2013 

======================================================================

ERROR: test_string_select (pandas.io.tests.test_pytables.TestHDFStore)

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/pandas-0.12.0_127_g46f6c43-py2.7-linux-x86_64.egg/pandas/io/tests/test_pytables.py", line 2424, in test_string_select

result = store.select('df',Term('x!=none'))

File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/pandas-0.12.0_127_g46f6c43-py2.7-linux-x86_64.egg/pandas/io/pytables.py", line 475, in select

raise KeyError('No object named %s in the file' % key)

KeyError: 'No object named df in the file'

@jreback
Copy link
Contributor

jreback commented Aug 7, 2013

its ok unrelated 2 u

@esc
Copy link
Contributor Author

esc commented Aug 12, 2013

So I am now skipping the unstable test. Problem is that Python 2.6 unittest module has no skip. What could/should I use as an alternative? Also the pytables test mentioned above is still failing. BTW: can/should I rebase this onto master?

@jreback
Copy link
Contributor

jreback commented Aug 12, 2013

yes rebase onto master

@jtratner
Copy link
Contributor

@esc just raise nose.SkipTest.

jtratner
jtratner reviewed Aug 13, 2013
View reviewed changes
pandas/io/tests/test_data.py
@@ -359,6 +359,7 @@ def test_read_famafrench(self):

class TestFred(unittest.TestCase):
@network
@unittest.skip('Skip as this is unstable #4427 ')
Copy link
Contributor

Choose a reason for hiding this comment

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

just change the first line of this function to be raise nose.SkipTest("Test is unstable #4427")

@jreback
Copy link
Contributor

jreback commented Aug 21, 2013

@esc how's this coming?

@esc
Copy link
Contributor Author

esc commented Sep 12, 2013

Okay, worked out how to skip the test properly. gonna rebase and then it'll be ready to merge.

esc added 2 commits September 12, 2013 16:40
@esc
BUG: off-by-one when doing network test repeats
c7ff981 
Imagine num_runs is two, then runs will take on the values [0, 1] and runs <
num_uns will always be False. Hence any genuine Exception the code might raise
will be silently ignored and just printed.
@esc
BUG/TST: fix (and skip) test that has been failing unnoticed all along
7f3823e 
The test is unstable and although this makes it work properly, no one knows how
long. Hence it was decided to skip this guy until further notice.

See also: #4427
@esc
Copy link
Contributor Author

esc commented Sep 12, 2013

Rebased onto current master, please merge.

@jtratner
Copy link
Contributor

@esc can you add a skip test to test_fred_nan and test_fred_parts? Then we can merge this. Going to open an issue that they need to be fixed.

jtratner added a commit that referenced this pull request Sep 20, 2013
@jtratner
Merge pull request #4485 'PR_fix_of-by-one-in-network-repeats'
02612c3 
Also added two skip tests for other fred tests in addition to @esc's
work.
@jtratner
Copy link
Contributor

Merged (I added two more skip tests for Fred). Thanks!

@jtratner jtratner closed this Sep 20, 2013
@esc
Copy link
Contributor Author

esc commented Sep 20, 2013

Thanks for adding the missing skips and for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13
Development

Successfully merging this pull request may close these issues.
4 participants
@esc @cpcloud @jreback @jtratner