Skip to content

Add a test for proper exception raising on bad start#25

Merged
petertodd merged 1 commit intopetertodd:masterfrom
icook:test-bad-start
Nov 20, 2014
Merged

Add a test for proper exception raising on bad start#25
petertodd merged 1 commit intopetertodd:masterfrom
icook:test-bad-start

Conversation

@icook
Copy link
Copy Markdown
Contributor

@icook icook commented Nov 16, 2014

A test to confirm the fix on icook@bdfc18a#commitcomment-8588990. It appears the actual issue is passing a string instead of bytes object in Python 2.7. I read several docs that stated "bytes in Python 2.7 is simply an alias for str" but in this case it appears to not behave quite the same. In the test I wrote on Python 2.7 if I remove the "b" before my string it raises a formatting exception. With the b it works fine. This is likely a systemic issue, and I'm not sure of a clever way to catch it and provide helpful exceptions to users... It seems silly/wasteful/painful to try and wrap all format bytes calls with b2x.

Comment thread bitcoin/tests/test_messages.py Outdated
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Seems you checked in a version of the test that you didn't actually try... this causes an uncaught ValueError.

Also, elsewhere I prefer to use the assertRaises with the with statement; more clear as to what exactly is going on.

Reminds me too: MsgSerializable should be raising a msg-specific exception class so serialization errors can be distinguished from unexpected ValueErrors. But I haven't actually had a chance to think through how that part of the API really should work.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Oh, and if you're not familiar, here's what I'm talking about:

with self.assertRaises(SomeException):
    do_something()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Defintiely more readable, I'd forgotten you could use it as a context manager.

@icook
Copy link
Copy Markdown
Contributor Author

icook commented Nov 18, 2014

Ah, that's emabarssing. I chopped my assertRaises briefly to cause to fail when I was testing it on various versions to make sure it was the "right" ValueError. Doh.

@petertodd petertodd merged commit 6d11a20 into petertodd:master Nov 20, 2014
petertodd added a commit that referenced this pull request Nov 20, 2014
6d11a20 Add a test for proper exception raising on bad start (Isaac Cook)
ghtdak pushed a commit to ghtdak/python-bitcoinlib that referenced this pull request Dec 1, 2015
6d11a20 Add a test for proper exception raising on bad start (Isaac Cook)

 [ yapified by gitreformat (github/ghtdak) on Mon Nov 30 21:11:00 2015 ]
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.

2 participants