zephyr icon indicating copy to clipboard operation
zephyr copied to clipboard

Add bluetooth host keys unit test

Open ahmedmoheb-nordic opened this issue 2 years ago • 5 comments

This is part of /host/key.c UT projects. Please review in order to catch review comments as early as possible

ahmedmoheb-nordic avatar Aug 04 '22 07:08 ahmedmoheb-nordic

@jori-nordic @kruithofa please review

ahmedmoheb-nordic avatar Aug 04 '22 07:08 ahmedmoheb-nordic

I see some of my comments could make it a pain to update all the other commits, and it will be hard to see if they're resolved across all the files (2000 lines changed).

Could we focus on the bt_keys_get_addr suites only in this PR (ie, could you remove the other commits), get it in good shape and merge it, and then review the rest? I think it'll be easier and faster.

Ok, let's do that.

ahmedmoheb-nordic avatar Sep 16 '22 16:09 ahmedmoheb-nordic

I see some of my comments could make it a pain to update all the other commits, and it will be hard to see if they're resolved across all the files (2000 lines changed).

Could we focus on the bt_keys_get_addr suites only in this PR (ie, could you remove the other commits), get it in good shape and merge it, and then review the rest? I think it'll be easier and faster.

As we agreed, I kept only the first 2 commits related to the mocks and bt_keys_get_addr test suites. Also, I added a set of fixup commits on top of that according to your review comments for easier changes tracking.

Please check and let me know.

ahmedmoheb-nordic avatar Sep 20 '22 05:09 ahmedmoheb-nordic

I think the PR looks much better now, comments are more descriptive, has been migrated to the new ztest API, etc. I only have some minor nits, and then we can hopefully merge.

jori-nordic avatar Sep 20 '22 07:09 jori-nordic

I think the PR looks much better now, comments are more descriptive, has been migrated to the new ztest API, etc. I only have some minor nits, and then we can hopefully merge.

I added some more fixup commits to address your latest comments.

ahmedmoheb-nordic avatar Sep 21 '22 05:09 ahmedmoheb-nordic