-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add tests for csp_crc32_verify #1
Conversation
be834d3
to
1afe4b6
Compare
src/crc32.c
Outdated
|
||
/* check error if length < sizeof(crc) */ | ||
ret = csp_crc32_verify(packet); | ||
zassert_equal(ret,CSP_ERR_CRC32, "Error on crc32_append"); |
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.
Is the error message valid?
Error on crc32_append
src/crc32.c
Outdated
packet->length += sizeof(crc); | ||
|
||
ret = csp_crc32_verify(packet); | ||
zassert_equal(0, ret); |
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.
It's better to use CSP_ERR_NONE
.
src/crc32.c
Outdated
/* Check without header */ | ||
csp_crc32_append(packet); | ||
ret = csp_crc32_verify(packet); | ||
zassert_equal(CSP_ERR_NONE, ret); |
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.
Is the error message unnecessary?
There is also a similar error message-less function called zasert_equal()
.
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.
Add the error message to all assert function.
src/crc32.c
Outdated
csp_packet_t *packet = fixture->buf; | ||
int ret; | ||
|
||
/* check error if length < sizeof(crc) */ |
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.
Isn't the purpose of this test to check that an error occurs if csp_crc32_append() is not called ?
If the purpose is to create “if length < sizeof(crc)”, the test below can be read as working
memcpy(packet->data, "Hello", 5);
packet->length = 5;
ret = csp_crc32_verify(packet);
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.
rename function name to test_crc_verify_without_crc
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.
Please update also comment.
/* check error if length < sizeof(crc) */
1afe4b6
to
2a98bde
Compare
src/crc32.c
Outdated
packet->length = 5; | ||
|
||
/* Check with header. | ||
* This simulate CSP_21 define on csp_crc32_append(). |
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.
Please format the code by zephry's clang-format.
Maybe you need to fix the below.
diff --git a/src/crc32.c b/src/crc32.c
index c206407..ec41a62 100644
--- a/src/crc32.c
+++ b/src/crc32.c
@@ -40,7 +40,7 @@ static void after(void *data)
ZTEST_F(crc, test_crc_verify_without_crc)
{
csp_packet_t *packet = fixture->buf;
- packet->length = sizeof(uint32_t)-1;
+ packet->length = sizeof(uint32_t) - 1;
int ret;
/* check error if length < sizeof(crc) */
@@ -72,8 +72,8 @@ ZTEST_F(crc, test_crc_with_header)
packet->length = 5;
/* Check with header.
- * This simulate CSP_21 define on csp_crc32_append().
- */
+ * This simulate CSP_21 define on csp_crc32_append().
+ */
csp_id_prepend(packet);
crc = csp_crc32_memory(packet->frame_begin, packet->frame_length);
/* Convert to network byte order */
2a98bde
to
b009065
Compare
src/crc32.c
Outdated
ZTEST_F(crc, test_crc_verify_without_crc) | ||
{ | ||
csp_packet_t *packet = fixture->buf; | ||
packet->length = sizeof(uint32_t) - 1; |
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.
Why set the sizeof(uint32_t) - 1
?
I think this setting is inconsistent with the test name without crc
.
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.
I was trying to test here : https://github.com/libcsp/libcsp/blob/develop/src/csp_crc32.c#L116-L118
But now I see that I'm not trying any more to test here : https://github.com/libcsp/libcsp/blob/develop/src/csp_crc32.c#L132-L134
Should I test the first error with the size part ? If not I will just go back to the previous code.
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.
I understand your purpose, but I think:
If you set the sizeof(uint32_t) - 1
, you need to set the 3 byte data to packet->data
.
Certainly, there is no point in setting data since it will be rejected by the length check, but I think the test code should comply with the original usage as much as possible.
And, one more things.
If we executes csp_crc32_verify()
without csp_crc32_append()
, as you say, there are two error routes.
- packet length less than 4 byte ( https://github.com/libcsp/libcsp/blob/develop/src/csp_crc32.c#L116-L118 )
- CRC value is unmatched ( https://github.com/libcsp/libcsp/blob/develop/src/csp_crc32.c#L132-L134 )
So in this test, we should be test the both case like below.
(It's just in example)
csp_packet_t *packet = fixture->buf;
packet->length = 0;
ret = csp_crc32_verify(packet);
zassert_equal(ret, CSP_ERR_CRC32, "Error on csp_crc32_verify");
memcpy(packet->data, "Hello", 5);
packet->length = 5;
ret = csp_crc32_verify(packet);
zassert_equal(ret, CSP_ERR_CRC32, "Error on csp_crc32_verify");
Anyway let's discuss later in F2F.
fd2c986
to
5728e96
Compare
src/crc32.c
Outdated
/* Check size error */ | ||
packet->length = 0; | ||
ret = csp_crc32_verify(packet); | ||
zassert_equal(ret, CSP_ERR_CRC32, "Error on csp_crc32_verify"); |
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.
Please add the causes to the content of the error messages so that it's clear which error is occured.
src/crc32.c
Outdated
memcpy(packet->data, "Hello", 5); | ||
packet->length = 5; | ||
ret = csp_crc32_verify(packet); | ||
zassert_equal(ret, CSP_ERR_CRC32, "Error on csp_crc32_verify"); |
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.
same as line 48.
This commit adds test for csp_crc32_verify . Signed-off-by: Gaetan Perrot <[email protected]>
5728e96
to
77f3a50
Compare
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.
Thanks, LGTM
This commit adds test for csp_crc32_verify .