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

Add tests for csp_crc32_verify #1

Merged
merged 1 commit into from Mar 26, 2024

Conversation

moonlight83340
Copy link
Contributor

This commit adds test for csp_crc32_verify .

@moonlight83340 moonlight83340 force-pushed the crc32_verify_test branch 2 times, most recently from be834d3 to 1afe4b6 Compare March 22, 2024 06:58
src/crc32.c Outdated Show resolved Hide resolved
src/crc32.c Outdated Show resolved Hide resolved
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");

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);

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);

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().

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 Show resolved Hide resolved
src/crc32.c Outdated
csp_packet_t *packet = fixture->buf;
int ret;

/* check error if length < sizeof(crc) */

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);

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

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) */

src/crc32.c Outdated Show resolved Hide resolved
src/crc32.c Show resolved Hide resolved
src/crc32.c Outdated
packet->length = 5;

/* Check with header.
* This simulate CSP_21 define on csp_crc32_append().

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 */

src/crc32.c Outdated
ZTEST_F(crc, test_crc_verify_without_crc)
{
csp_packet_t *packet = fixture->buf;
packet->length = sizeof(uint32_t) - 1;

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.

Copy link
Contributor Author

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.

Copy link

@sasataku sasataku Mar 25, 2024

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.

  1. packet length less than 4 byte ( https://github.com/libcsp/libcsp/blob/develop/src/csp_crc32.c#L116-L118 )
  2. 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.

@moonlight83340 moonlight83340 force-pushed the crc32_verify_test branch 2 times, most recently from fd2c986 to 5728e96 Compare March 26, 2024 05:09
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");

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");

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 <gaetan.perrot@spacecubics.com>
Copy link

@sasataku sasataku left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@yashi yashi self-requested a review March 26, 2024 09:42
@yashi yashi merged commit 6e41823 into libcsp:main Mar 26, 2024
3 checks passed
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.

None yet

3 participants