Skip to content
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

Added hypothesis property based testing #264

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Added hypothesis property based testing #264

wants to merge 14 commits into from

Conversation

thedrow
Copy link
Member

@thedrow thedrow commented Mar 9, 2019

This PR uses hypothesis to check if arrays & dictionaries are serialized and deserialized correctly.

I also fixed our integer serialization and deserialization code, bytes string serialization and small string serialization on tables and arrays.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

REALLY TOUGH TO REVIEW SUCH LARGE DIFF!!

@thedrow
Copy link
Member Author

thedrow commented Mar 10, 2019

Yes, the examples directory should be ignored when reviewing this.

@thedrow
Copy link
Member Author

thedrow commented Mar 12, 2019

Seems like we have a genuine problem with strings inside dictionaries on Python 2.
Even without any adjustments that were made in this branch I can reproduce this:

Falsifying example: test_table(self=<t.unit.test_serialization.test_serialization instance at 0x7fdf17cde518>, table={u'': u'\x80'})
Traceback (most recent call last):
  File "/home/omer/Documents/Projects/py-amqp/t/unit/test_serialization.py", line 126, in test_table
    assert loads(b'F', dumps(b'F', [table]))[0][0] == table
AssertionError: assert {'': '\xc2\x80'} == {'': '\x80'}
  Differing items:
  {'': '\xc2\x80'} != {'': '\x80'}
  Full diff:
  - {'': '\xc2\x80'}
  ?       ----
  + {u'': u'\x80'}
  ?  +    +

@thedrow
Copy link
Member Author

thedrow commented Mar 12, 2019

Since '\x80' is a unicode string when you encode it to bytes, you get \xc2\x80 but we aren't decoding back. We're encoding again.

Not sure how to resolve this issue.

@thedrow thedrow requested a review from auvipy March 13, 2019 14:10
@thedrow
Copy link
Member Author

thedrow commented Mar 13, 2019

Authentication fails with the addition of support for small strings in tables and arrays.
I'm not sure why though.

Also, tox-docker doesn't seem to work anymore which is super strange.

@thedrow
Copy link
Member Author

thedrow commented Mar 13, 2019

Seems like there's a bug in RabbitMQ with short strings in tables.
It is allowed in the spec.

@thedrow
Copy link
Member Author

thedrow commented Mar 14, 2019

I found the source of the problem:
When RabbitMQ authenticates using the AMQPLAIN method it expects a longstr instead of a shortstr.
See issue rabbitmq/rabbitmq-server#1914.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

IMHO te existing example based tests should stay on place. we should use property based testing as a complete different suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants