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

Write more tests that test most-aspects of message (de)serialization #1263

Open
stebet opened this issue Oct 13, 2022 · 3 comments
Open

Write more tests that test most-aspects of message (de)serialization #1263

stebet opened this issue Oct 13, 2022 · 3 comments
Assignees
Milestone

Comments

@stebet
Copy link
Contributor

stebet commented Oct 13, 2022

To prevent more bugs like #1260 slipping through we should have more thorough integration and unit tests that test most aspects of message (de)serialization.

Ideas:

  • Tests that use most properties from BasicProperties and insert random string, number and binary values into the BasicProperties.Headers dictionary and make sure they are sent and received as expected.
  • Write proper unit-tests for individual Frame (de)serializations and parallelize them to make sure nothing unexpected happens where we might be dipping down into unsafe code. WireFormatting.WriteLongstr comes to mind.
  • More?

CCing @bollhals for added input :)

@michaelklishin
Copy link
Member

Property-based tests should be very helpful for issues like #1260. I am not familiar with the state of property-based testing in .NET, though.

@bollhals
Copy link
Contributor

Oh wow, yes, it seems we're definitely lacking some coverage somewhere. I'm somehow relied on the coverage to spot any bugs when I was working on #1096.

This should be investigated. I'm currently a bit out of the loop about what kind of test we have for the properties, I think we have full fledged tests for the serialization / deserialization, but as it seems we don't have specific tests that tests them for a full round trip.

@stebet
Copy link
Contributor Author

stebet commented Oct 13, 2022

The deserialization was broken, but it broke without throwing an exception, since it was reading incorrect bytes determining the Property frame size, which always resulted it thinking it had no Property content (always read zeros). So Properties, although they were received and sent correctly, they weren't deserialized correctly in the client.

Or more accurately, they weren't deserialized at all since it though the Property content size was 0 and didn't do anything.

@lukebakken lukebakken self-assigned this Nov 18, 2023
@lukebakken lukebakken added this to the 7.0.0 milestone Nov 18, 2023
@lukebakken lukebakken modified the milestones: 7.0.0, 8.0.0 Jun 3, 2024
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

No branches or pull requests

4 participants