Skip to content

Added hypothesis property based testing#264

Open
thedrow wants to merge 14 commits intomainfrom
hypothesis
Open

Added hypothesis property based testing#264
thedrow wants to merge 14 commits intomainfrom
hypothesis

Conversation

@thedrow
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor Author

thedrow commented Mar 10, 2019

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

@thedrow
Copy link
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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
Copy Markdown
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.

3 participants