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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(txt) initial unit tests and general code cleanup/fixes #2623

Merged
merged 23 commits into from
Oct 15, 2021

Conversation

C47D
Copy link
Contributor

@C47D C47D commented Sep 29, 2021

Description of the feature or fix

What the title says, if you need me to add any special test please let me know 馃樃

Checkpoints

@C47D C47D marked this pull request as draft September 29, 2021 23:23
@C47D
Copy link
Contributor Author

C47D commented Oct 1, 2021

For this tests it would be nice to test the decoders with some strings? What do you think?

@kisvegabor
Copy link
Member

For this tests it would be nice to test the decoders with some strings? What do you think

Yes, it'd be great. IMO these functions are well-testable:

  • _lv_txt_encoded_next
  • _lv_txt_cut
  • _lv_txt_ins

@C47D
Copy link
Contributor Author

C47D commented Oct 8, 2021

I'm writing a test for _lv_txt_ins, it's using the lv_txt_utf8_get_byte_id function, is there a way to also test the LV_TXT_ENC_ASCII static functions?

Also, I think this patch needs to be added to _lv_txt_ins, passing NULL to strlen seems to be undefined behavior.
EDIT
We should also add a note about the _lv_txt_ins parameters txt_buf and ins_txt, both must be NULL terminated, otherwise we get undefined behavior when calling strlen on them.

diff --git a/src/misc/lv_txt.c b/src/misc/lv_txt.c
index ca51e0b81..4d586d23d 100644
--- a/src/misc/lv_txt.c
+++ b/src/misc/lv_txt.c
@@ -440,6 +440,8 @@ bool _lv_txt_is_cmd(lv_text_cmd_state_t * state, uint32_t c)
  */
 void _lv_txt_ins(char * txt_buf, uint32_t pos, const char * ins_txt)
 {
+    if(txt_buf == NULL || ins_txt == NULL) return;
+
     size_t old_len = strlen(txt_buf);
     size_t ins_len = strlen(ins_txt);
     if(ins_len == 0) return;

Passing NULL to strlen is not defined, so we should avoid it
Remove docs from source file and add comment about pointers to NULL terminated arrays where necessary
@C47D C47D changed the title txt: initial unit tests txt: initial unit tests and genetic improvements Oct 9, 2021
@C47D C47D changed the title txt: initial unit tests and genetic improvements txt: initial unit tests and generic fixes Oct 9, 2021
@C47D
Copy link
Contributor Author

C47D commented Oct 9, 2021

I've added a comment about _lv_txt_cut in test_txt_cut_len_longer_than_string, is that the expected behavior?

src/misc/lv_txt.h Outdated Show resolved Hide resolved
src/misc/lv_txt.h Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
@C47D
Copy link
Contributor Author

C47D commented Oct 12, 2021

I will test your new commit and revert the pragma one later today @embeddedt

@C47D
Copy link
Contributor Author

C47D commented Oct 12, 2021

I've added a comment about _lv_txt_cut in test_txt_cut_len_longer_than_string, is that the expected behavior?

If so we can add a note about it in the docs.

@C47D
Copy link
Contributor Author

C47D commented Oct 13, 2021

_lv_txt_encoded_next test inputs from here https://stackoverflow.com/a/3886015 , let me know what do you think

@C47D C47D marked this pull request as ready for review October 13, 2021 03:50
@C47D C47D marked this pull request as draft October 13, 2021 16:58
@C47D C47D marked this pull request as ready for review October 13, 2021 18:16
src/misc/lv_txt.c Outdated Show resolved Hide resolved
@embeddedt embeddedt changed the title txt: initial unit tests and generic fixes test(txt) initial unit tests and general code cleanup/fixes Oct 14, 2021
@C47D C47D mentioned this pull request Oct 15, 2021
3 tasks
Copy link
Member

@kisvegabor kisvegabor left a comment

Choose a reason for hiding this comment

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

Looks good, thank you very much!

@kisvegabor kisvegabor merged commit 42989d4 into lvgl:master Oct 15, 2021
@C47D C47D deleted the test/txt_initial_tests branch October 15, 2021 22:41
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