Skip to content

Conversation

@raperez2
Copy link
Contributor

@raperez2 raperez2 commented May 8, 2024

Added some tests for ByteBuf, they probably don't work. Is there anything I can do to fix them?

Added some tests for ByteBuf, they probably don't work. How can I change that?
Copy link
Contributor

@Tabris05 Tabris05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments on specific lines of code pointing out some specific mistakes. One other more general thing is that the naming convention we've been following for tests is that the first label in the TEST macro specifies the function you are testing and the second label explains what you are doing specifically (eg for StrA: TEST(strcmp, ZeroLengthString))

ByteBuf test;
U8 testValue = 'a';
U8* testdata = &testValue;
U32 dataSize = sizeof(testdata);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you want to take the size of your underlying data rather than the size of the pointer

U8 testValue = 'a';
U8* testdata = &testValue;
U32 dataSize = sizeof(testdata);
result = test.write_bytes(testdata, dataSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no need to make a copy of test here since write bytes operates on the original buffer

U8* testdata = &testValue;
U32 dataSize = sizeof(testdata);
result = test.write_bytes(testdata, dataSize);
EXPECT_EQ(test.bytes, result);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not the right comparison to make. You'd want to ensure test.bytes contains the same data as the testValue you put inside it

U8* testdata = &testValue;
U32 dataSize = sizeof(testdata);
test.wrap(testdata, dataSize);
EXPECT_EQ(test.bytes, 'a');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since test.bytes is a pointer this will check if the address of the byte buffer is equal to the numerical ascii code for a. You need some more custom code to validate that the contents of the buffer is correct

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that the wrap method literally makes a wrapper around the pointer you pass in. In other words, test.bytes == testdata, so that's what you'd want to check if you're testing the wrap method.

Copy link
Contributor

@Drillgon200 Drillgon200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found some other issues that should be addressed.

U8* testdata = &testValue;
U32 dataSize = sizeof(testdata);
test.wrap(testdata, dataSize);
EXPECT_EQ(test.bytes, 'a');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that the wrap method literally makes a wrapper around the pointer you pass in. In other words, test.bytes == testdata, so that's what you'd want to check if you're testing the wrap method.

U32 dataSize = sizeof(testdata);
test.wrap(testdata, dataSize);
test.skip(1);
EXPECT_EQ(test.bytes, 'a');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entire sure what's being tested here. You could check that test.bytes is unmodified after the skip, or that it did indeed skip one byte, resulting in a zero length buffer.


TEST(ByteBuff, write_bytes) {
ByteBuf test;
ByteBuf result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason write_bytes returns a ByteBuf& is so that you can chain function calls together. For example,
buf.write_f32(...).write_bytes(...).write_bytes(...).write_stra("myString"sa);
I don't think it's particularly useful to assign the result to another variable, since all that does is make an extra copy.

ByteBuf test;
ByteBuf result;
result = test.write_stra(str);
EXPECT_EQ(test.bytes, result);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setup is wrong, and this comparison makes little sense. If you want to test serialization of a string, you'll have to give the ByteBuf a buffer to serialize into (wrap a U8[1024], for example). To test write_stra, inspect the byte buffer to make sure that it contains the string length in 32 bit little endian followed by the string's content, exactly as long as the preceding length.

U8 testValue = 'a';
U8* testdata = &testValue;
U32 dataSize = sizeof(testdata);
test.read_bytes(testdata, dataSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't read it into the same memory location the buffer is using in the first place. An empty implementation of read_bytes would pass that. Instead, read the bytes into a different block of memory.

U8* testdata = &testValue;
U32 dataSize = sizeof(testdata);
test.wrap(testdata, dataSize);
EXPECT_TRUE(test.has_data_left(dataSize));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While good for a start, I would add a little more logic to help ensure has_data_left works correctly. For example, wrap a larger buffer, test that it has the full amount left, read n bytes, then test that it has bufferSize - n bytes left and that it doesn't have bufferSize - n + 1 bytes left.

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.

4 participants