rabbitmq / rabbitmq-dotnet-client

RabbitMQ .NET client for .NET Standard 2.0+ and .NET 4.6.2+
https://www.rabbitmq.com/dotnet.html
Other
2.07k stars 576 forks source link

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

Open stebet opened 1 year ago

stebet commented 1 year ago

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:

CCing @bollhals for added input :)

michaelklishin commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.