test: ztest: Add fast-get unit tests converted from CMock #10136
Conversation
tmleman
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR converts legacy CMock-based fast-get unit tests to Zephyr's Ztest framework, maintaining equivalent test coverage while modernizing the testing approach. The conversion includes four main test functions that validate fast-get library functionality including basic operations, error handling, multiple allocations, and reference counting.
- Converts four CMock/Unity tests to Ztest framework with identical functionality
- Implements mock memory allocation functions using standard malloc/free
- Adds proper Ztest configuration and build setup for the fast-get module
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| test/ztest/unit/fast-get/testcase.yaml | Test configuration defining platform requirements and test metadata |
| test/ztest/unit/fast-get/test_fast_get_ztest.c | Main test implementation with converted test cases and mock functions |
| test/ztest/unit/fast-get/prj.conf | Zephyr project configuration enabling Ztest framework |
| test/ztest/unit/fast-get/CMakeLists.txt | CMake build configuration with SOF integration and function wrapping |
| { 18 }, | ||
| { 19 }, | ||
| { 20 }, | ||
| { 21 }, |
There was a problem hiding this comment.
The testdata array has a gap in the sequence - it jumps from 21 to 23, skipping 22. This could indicate a copy-paste error or intentional omission that should be documented.
| { 21 }, | |
| { 21 }, | |
| { 22 }, |
There was a problem hiding this comment.
@tmleman If intentional skip, this would warrant a comment.
There was a problem hiding this comment.
The data comes from the original test: https://github.com/thesofproject/sof/blob/main/test/cmocka/src/lib/fast-get/fast-get-tests.c#L54-L57
I'll check the history of those tests to see if I can find any justification for this. There's nothing in the code.
There was a problem hiding this comment.
@kv2019i git didn't tell me anything interesting. The test has looked like this from the beginning, and there doesn't seem to be any hidden meaning behind it.
There was a problem hiding this comment.
Ack @tmleman . It looks like a bug, but maybe fix in a follow-up PR and move the test code as-is in this one.
| * Tests that fast_get returns NULL when requested size doesn't match | ||
| * previously allocated size for the same data pointer. | ||
| */ | ||
| ZTEST(fast_get_suite, test_fast_get_size_missmatch_test) |
There was a problem hiding this comment.
Function name contains a spelling error: 'missmatch' should be 'mismatch'.
| ZTEST(fast_get_suite, test_fast_get_size_missmatch_test) | |
| ZTEST(fast_get_suite, test_fast_get_size_mismatch_test) |
| copy[i] = fast_get(testdata[i], sizeof(testdata[0])); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy); i++) | ||
| zassert_mem_equal(copy[i], testdata[i], sizeof(testdata[0]), |
There was a problem hiding this comment.
Using sizeof(testdata[0]) for all array elements is incorrect. testdata[0] has 100 elements while testdata[1] through testdata[32] only have 1 element each. This should use sizeof(testdata[i]) to get the correct size for each test data array.
| copy[i] = fast_get(testdata[i], sizeof(testdata[0])); | |
| for (i = 0; i < ARRAY_SIZE(copy); i++) | |
| zassert_mem_equal(copy[i], testdata[i], sizeof(testdata[0]), | |
| copy[i] = fast_get(testdata[i], sizeof(testdata[i])); | |
| for (i = 0; i < ARRAY_SIZE(copy); i++) | |
| zassert_mem_equal(copy[i], testdata[i], sizeof(testdata[i]), |
There was a problem hiding this comment.
copilot forgot which language it's reviewing. False positive.
| copy[i] = fast_get(testdata[i], sizeof(testdata[0])); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy); i++) | ||
| zassert_mem_equal(copy[i], testdata[i], sizeof(testdata[0]), |
There was a problem hiding this comment.
Using sizeof(testdata[0]) for memory comparison is incorrect for elements beyond testdata[0]. This will read beyond the bounds of testdata[1] through testdata[32] which only contain 1 element each. Should use sizeof(testdata[i]).
| copy[i] = fast_get(testdata[i], sizeof(testdata[0])); | |
| for (i = 0; i < ARRAY_SIZE(copy); i++) | |
| zassert_mem_equal(copy[i], testdata[i], sizeof(testdata[0]), | |
| copy[i] = fast_get(testdata[i], sizeof(testdata[i])); | |
| for (i = 0; i < ARRAY_SIZE(copy); i++) | |
| zassert_mem_equal(copy[i], testdata[i], sizeof(testdata[i]), |
| copy[0][i] = fast_get(testdata[i], sizeof(testdata[0])); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | ||
| copy[1][i] = fast_get(testdata[i], sizeof(testdata[0])); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | ||
| zassert_equal_ptr(copy[0][i], copy[1][i], | ||
| "Same data should return same pointer (refcounting)"); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | ||
| zassert_mem_equal(copy[0][i], testdata[i], sizeof(testdata[0]), |
There was a problem hiding this comment.
Using sizeof(testdata[0]) for all elements is incorrect. Should use sizeof(testdata[i]) to get the correct size for each individual test data array.
| copy[0][i] = fast_get(testdata[i], sizeof(testdata[0])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| copy[1][i] = fast_get(testdata[i], sizeof(testdata[0])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_equal_ptr(copy[0][i], copy[1][i], | |
| "Same data should return same pointer (refcounting)"); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_mem_equal(copy[0][i], testdata[i], sizeof(testdata[0]), | |
| copy[0][i] = fast_get(testdata[i], sizeof(testdata[i])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| copy[1][i] = fast_get(testdata[i], sizeof(testdata[i])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_equal_ptr(copy[0][i], copy[1][i], | |
| "Same data should return same pointer (refcounting)"); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_mem_equal(copy[0][i], testdata[i], sizeof(testdata[i]), |
| copy[0][i] = fast_get(testdata[i], sizeof(testdata[0])); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | ||
| copy[1][i] = fast_get(testdata[i], sizeof(testdata[0])); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | ||
| zassert_equal_ptr(copy[0][i], copy[1][i], | ||
| "Same data should return same pointer (refcounting)"); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | ||
| zassert_mem_equal(copy[0][i], testdata[i], sizeof(testdata[0]), |
There was a problem hiding this comment.
Using sizeof(testdata[0]) for all elements is incorrect. Should use sizeof(testdata[i]) to get the correct size for each individual test data array.
| copy[0][i] = fast_get(testdata[i], sizeof(testdata[0])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| copy[1][i] = fast_get(testdata[i], sizeof(testdata[0])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_equal_ptr(copy[0][i], copy[1][i], | |
| "Same data should return same pointer (refcounting)"); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_mem_equal(copy[0][i], testdata[i], sizeof(testdata[0]), | |
| copy[0][i] = fast_get(testdata[i], sizeof(testdata[i])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| copy[1][i] = fast_get(testdata[i], sizeof(testdata[i])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_equal_ptr(copy[0][i], copy[1][i], | |
| "Same data should return same pointer (refcounting)"); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_mem_equal(copy[0][i], testdata[i], sizeof(testdata[i]), |
| copy[0][i] = fast_get(testdata[i], sizeof(testdata[0])); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | ||
| copy[1][i] = fast_get(testdata[i], sizeof(testdata[0])); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | ||
| zassert_equal_ptr(copy[0][i], copy[1][i], | ||
| "Same data should return same pointer (refcounting)"); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | ||
| zassert_mem_equal(copy[0][i], testdata[i], sizeof(testdata[0]), |
There was a problem hiding this comment.
Using sizeof(testdata[0]) for memory comparison will cause buffer overread for testdata[1] through testdata[32]. Should use sizeof(testdata[i]).
| copy[0][i] = fast_get(testdata[i], sizeof(testdata[0])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| copy[1][i] = fast_get(testdata[i], sizeof(testdata[0])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_equal_ptr(copy[0][i], copy[1][i], | |
| "Same data should return same pointer (refcounting)"); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_mem_equal(copy[0][i], testdata[i], sizeof(testdata[0]), | |
| copy[0][i] = fast_get(testdata[i], sizeof(testdata[i])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| copy[1][i] = fast_get(testdata[i], sizeof(testdata[i])); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_equal_ptr(copy[0][i], copy[1][i], | |
| "Same data should return same pointer (refcounting)"); | |
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | |
| zassert_mem_equal(copy[0][i], testdata[i], sizeof(testdata[i]), |
|
|
||
| /* Data should still be valid through second set of references */ | ||
| for (i = 0; i < ARRAY_SIZE(copy[0]); i++) | ||
| zassert_mem_equal(copy[1][i], testdata[i], sizeof(testdata[0]), |
There was a problem hiding this comment.
Using sizeof(testdata[0]) for memory comparison will cause buffer overread for testdata[1] through testdata[32]. Should use sizeof(testdata[i]).
| zassert_mem_equal(copy[1][i], testdata[i], sizeof(testdata[0]), | |
| zassert_mem_equal(copy[1][i], testdata[i], sizeof(testdata[i]), |
Convert legacy CMock-based fast-get unit tests to Zephyr Ztest framework. This patch converts the existing fast-get unit tests from CMock/Unity to Zephyr's Ztest framework, maintaining the same test coverage and functionality: - test_simple_fast_get_put: Basic fast_get/fast_put functionality - test_fast_get_size_missmatch_test: Size mismatch error handling - test_over_32_fast_gets_and_puts: Multiple allocation handling (>32) - test_fast_get_refcounting: Reference counting validation The converted tests validate the same fast-get library functionality as the original CMock tests, ensuring no regression in test coverage during the migration to Ztest framework. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
is it possible and do we plan to enable this in PR CI as a github worker?
| copy[i] = fast_get(testdata[i], sizeof(testdata[0])); | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(copy); i++) | ||
| zassert_mem_equal(copy[i], testdata[i], sizeof(testdata[0]), |
There was a problem hiding this comment.
copilot forgot which language it's reviewing. False positive.
false alarm - it's already running https://github.com/thesofproject/sof/actions/runs/16503100878/job/46666728466?pr=10136 |
| { 18 }, | ||
| { 19 }, | ||
| { 20 }, | ||
| { 21 }, |
There was a problem hiding this comment.
@tmleman If intentional skip, this would warrant a comment.
3 participants
Convert legacy CMock-based fast-get unit tests to Zephyr Ztest framework.
This patch converts the existing fast-get unit tests from CMock/Unity to Zephyr's Ztest framework, maintaining the same test coverage and functionality:
The converted tests validate the same fast-get library functionality as the original CMock tests, ensuring no regression in test coverage during the migration to Ztest framework.