Skip to content

Conversation

@rwarren
Copy link

@rwarren rwarren commented Feb 7, 2018

This is to ensure that behaviour referenced in #281 doesn't break in the future.

@methane
Copy link
Member

methane commented Feb 8, 2018

Have you checked there are no test about that for now?

@rwarren rwarren changed the title added test case "test_bintype_symmetry" (see issue #281) added test case "test_mixed_bin_str" (see issue #281) Feb 8, 2018
@rwarren
Copy link
Author

rwarren commented Feb 8, 2018

Have you checked there are no test about that for now?

Yes. I could find nothing that explicitly, or properly, checks dealing with mixed bin/str elements in MessagePack protocol stream.


More detail...

A proper test for this requires:

  1. testing a raw MessagePack byte sequence with mixed bin/str types in it
  2. explicit testing of the symmetry when using use_bin_type=True and raw=False
  3. assertion of identical python types (not just equality testing with ==)
  4. testing properly under both python 2 and python 3

The closest I could find (that do not meet these criteria) are:

  • tests in test_case.py
    • No tests for mixed bin/str types. They also don't test any use_bin_type usage. They also use equality testing with == which does not ensure type matches.
  • test_stricttype.test_tuple seems like, at first glance, that it kind of checks it... but it doesn't meet the criteria
    • it does partially cover the condition under python 3 (the right pack/unpack sequence happens to happen there), but there is no explicit byte stream specified, and it doesn't meet several other other criteria (eg: no type checking, doesn't properly cover python2, etc). In addition, the fact that it happens to do a piece of the test is mostly by accident, since it is not the purpose of the test.
  • test_pack.py and test_unpack.py also don't have anything that properly tests it

With a lack of an explicit test, I thought one should be added, so I wrote it rather than requested it. Note that I also considered writing a test case with multiple cases (similar to what test_case.test_match does), but ended up thinking that a clean single case test for the specific conditions was better.

Also - I renamed the test to better reflect what it does. The original name wasn't great.

@jfolz
Copy link
Contributor

jfolz commented Feb 10, 2018

Mixed testing should not be necessary. Packing/unpacking does not change internal state of the Packer/Unpacker beyond managing message buffers. As long as each object/token is handled correctly any mix of types will be handled correctly.
That said there are some issues with the current test suite. E.g., this assert test_pack.py#L15 is not triggered if given str and unicode in Python 2.7. This type of assert is used in several tests.
Also, a lot of tests use some form of packing-unpacking chain. Regressions in the Packer can be hidden by regressions in the Unpacker. They should probably be rewritten in the style of test_format.py: specifying the ground-truth data-object pair as per msgpack spec that to be handled correctly.

@methane methane closed this Oct 3, 2018
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.

3 participants