Skip to content

RFC: Expand details API #774

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

Open
ml-physec opened this issue Apr 3, 2025 · 2 comments
Open

RFC: Expand details API #774

ml-physec opened this issue Apr 3, 2025 · 2 comments

Comments

@ml-physec
Copy link

ml-physec commented Apr 3, 2025

Currently there are only two hardcoded "slots" for detail, which are generally used for "Function" and "Argument" by CMock.
The details are also inside unity_internals.h, so mostly hidden to regular users.

I was thinking of submitting a PR expanding this to be a public-facing, stack-based API, to avoid having to pre-construct long strings (to use as message) when wanting to locate an error further, e.g. Section first_iteration, Function do_stuff, Argument list_of_structs, Element 4, Member value.

In addition to UnityAssertEqual_ implementations allowing more granular information than "some HEX16 in that argument is wrong", I see this also being useful a variety of other applications, like labeling parametrized tests and sections (not just inside the test itself, but also setup/teardown), and simplifying some of the existing functions (e.g. UnityAssertEqual*Array).

Implementation thoughts:

  • Implement an array of strings, mapping "detail id" to human-readable, with the default value being {UNITY_DETAIL1_NAME, UNITY_DETAIL2_NAME} (i.e. "Function", "Argument"). Additional id strings may be "Element", "Member", "Byte", "Bit", "File", "Line", "Section", "Case".
  • Limit the number of stack entries to keep UNITY_STORAGE_T size low (configured with define, with 2 being the default)
  • Stack should store "detail id" (uint8_t should be enough) and value (char*). For maximum memory efficiency, this could be two arrays in UNITY_STORAGE_T, and one counter. In the default settings with 2 slots, this would only increase the size of UNITY_STORAGE_T by 3-6 bytes (1-4 for stack counter (UNITY_COUNTER_TYPE), 2x1 for detail-ids. The pointers already exist for the current slots).
  • Potentially some way to mark some details as numeric (with value being interpreted as uintptr_t instead of char*), to support array/line numbers without string-conversion. Maybe end the string with a # or some other marker?
  • To retain backwards-compatiblity, UNITY_SET_DETAIL, UNITY_SET_DETAILS should keep their current functionality, but pushing to the detail stack instead of using hardcoded slots, and UNITY_CLR_DETAILS should be able to pop either the single "detail1", or both.

Please let me know if you would consider accepting such a PR (and a potential follow-up to CMock for better integration there), or have any feedback.

@mvandervoord
Copy link
Member

I'd love to have an alternative method which provides more detail... particular a stack-trace-like output. I should stress that it should be an optional alternative. There are a lot of people using CMock on very memory constrained platforms. In these situations, the current method (which is really a pair of pointers to static strings) is about as light as we can make it. So this functionality should be preserved.

Having another option, however, is excellent.

@ml-physec
Copy link
Author

ml-physec commented Apr 4, 2025

Stacktrace-like output would be possible, when adding push&pops inside the application code at key calls (which would compile to no-ops for non-test builds):

// either by callee
int my_func(int some_value) {
    DETAIL_PUSH(DETAIL_ID_CALL, __FUNC__);
    DETAIL_PUSH(DETAIL_ID_SOME_VALUE, (uintptr_t)value);
    ...
    DETAIL_POP(DETAIL_ID_SOME_VALUE, (uintptr_t)value);  // repeat args to detect missing POPs
    DETAIL_POP(DETAIL_ID_CALL, __FUNC__);
    return ret;
}
// or by caller
...
int call_intermediate() {
    ...
    DETAIL_PUSH(DETAIL_ID_CALL, MY_FUNC_NAME);
    DETAIL_PUSH(DETAIL_ID_SOME_VALUE, (uintptr_t)value);
    ret = my_func(value);
    DETAIL_POP(DETAIL_ID_SOME_VALUE, (uintptr_t)value);
    DETAIL_POP(DETAIL_ID_CALL,  MY_FUNC_NAME);
    ...
}
...
// example formatted output (existing format order: header->assert->detail->message, linebreaks added for readability)
some_test.c:123:test_dispatcher_intermediate_myfunc_happypath:FAIL: Expected 0x0001 Was 0x0000.
Call dispatcher_func Operation call_intermediate Call intermediate_func Call my_func Some Value 123 Function do_operation Argument bit_mask.
Function called with unexpected argument value.
// suggested new ordering (header->detail->message->assert)
some_test.c:123:test_dispatcher_intermediate_myfunc_happypath:FAIL:
Call dispatcher_func, Operation call_intermediate, Call intermediate_func, Call my_func, Some Value 123, Function do_operation, Argument bit_mask.
Function called with unexpected argument value. Expected 0x0001 Was 0x0000.

I am aware of the memory constrained nature.
While I aim to make the final implementation as compact as possible such that it could replace the existing system with negligible overhead, I would keep the existing detail system the default until further notice for backwards compatibility.
This will also allow comparing both implementations and letting users determine the tradeoff themselves.

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

No branches or pull requests

2 participants