-
Notifications
You must be signed in to change notification settings - Fork 103
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
Set NULL payloads and fix unit test #71
Conversation
682f055
to
432bf1c
Compare
432bf1c
to
03deda8
Compare
publishInfo.pTopicName = TEST_TOPIC_NAME; | ||
publishInfo.topicNameLength = TEST_TOPIC_NAME_LENGTH; | ||
publishInfo.pPayload = MQTT_SAMPLE_PAYLOAD; | ||
publishInfo.payloadLength = MQTT_SAMPLE_PAYLOAD_LEN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you possibly use setupWillInfo
for these instead, changing what is necessary for the specific test case? Maybe using the built in unity fixture, setUp
, can make this easier for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll rename it to setupPublishInfo
pPublishInfo->payloadLength = ( pIncomingPacket->remainingLength - pPublishInfo->topicNameLength - 2U * sizeof( uint16_t ) ); | ||
pPublishInfo->pPayload = pPacketIdentifierHigh + sizeof( uint16_t ); | ||
/* Two more bytes for the packet identifier. */ | ||
pPublishInfo->payloadLength -= sizeof( uint16_t ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding two bytes in advance then subtracting them if pPublishInfo->qos != MQTTQoS0
, why not just add once if pPublishInfo->qos != MQTTQoS0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean, we're not adding any bytes to the payload length. The payload length is defined as: Remaining length - # of Topic length bytes - Topic length - # of packet ID bytes. There are two packet ID bytes for QoS > 0, and none for QoS 0. So, we need to subtract an additional 2 for QoS > 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh got it, I seem to have misread the code.
TEST_ASSERT_EQUAL_INT( TEST_TOPIC_NAME_LENGTH, publishInfo.topicNameLength ); | ||
TEST_ASSERT_EQUAL_MEMORY( TEST_TOPIC_NAME, publishInfo.pTopicName, TEST_TOPIC_NAME_LENGTH ); | ||
TEST_ASSERT_EQUAL_INT( MQTT_SAMPLE_PAYLOAD_LEN, publishInfo.payloadLength ); | ||
TEST_ASSERT_EQUAL_MEMORY( MQTT_SAMPLE_PAYLOAD, publishInfo.pPayload, MQTT_SAMPLE_PAYLOAD_LEN ); | ||
|
||
memset( ( void * ) &mqttPacketInfo, 0x00, sizeof( mqttPacketInfo ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the cast to void * should not be required and looks strange to me. The point of a void * is to be a permissive pointer that accepts whatever it gets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
Description:
Set payload to NULL for zero length payloads:
MQTT_DeserializePublish
setspPayload
to point to the byte after the topic name (or packet ID for QoS > 0). If there is no payload, then this could point to garbage data in the buffer.payloadLength
is correctly set to 0 for this case, but it also makes sense to set the payload to NULL as well.Improve unit tests for MQTT_DeserializePublish:
MQTT_DeserializePublish
don't reset itspublishInfo
struct between test cases. Before a test, its pointers point to constant strings, but are changed to point to the serialized buffer afterwards. This is an issue for the next test as it will try to serialize the data into the same buffer that is being pointed to, and mangles the data.