feat: Move test status to header file.#341
feat: Move test status to header file.#341SapthagiriP wants to merge 1 commit intoARM-software:mainfrom
Conversation
- make the test status to be used in
both VAL and PAL layer
Signed-off-by: sapthagiri padmanabhan <sapthagiri.padmanabhan@arm.com>
Change-Id: I987440785dc374780f948e39f993c9bd03eac753
prashymh
left a comment
There was a problem hiding this comment.
As discussed, this needs some clean up
The issue is that a mutable global singleton is being exposed directly from a public header at acs_test_status.h (line 33), then written from rule_based_execution_helpers.c (line 270) and read from acs_test_infra.c (line 148). That works, but it increases coupling and makes resets/ownership easy to get wrong.
A better implementation here is to keep the struct type public, but hide the storage in one .c file and expose small APIs instead:
typedef struct {
uint32_t total_rules_run;
uint32_t passed;
uint32_t partial_coverage;
uint32_t warnings;
uint32_t skipped;
uint32_t failed;
uint32_t not_implemented;
uint32_t pal_not_supported;
} acs_test_status_counters_t;
void acs_test_status_reset(void);
void acs_test_status_record(uint32_t status, uint32_t indent);
const acs_test_status_counters_t *acs_test_status_get(void);
That is better because:
only one module owns mutation rules
PAL/VAL callers do not poke fields directly
reset logic becomes explicit instead of implicit
you can later add locking or per-run context without changing all call sites
If PAL only needs to read the counters, return const acs_test_status_counters_t * or a copy-by-value snapshot. If PAL also needs to update them, still prefer acs_test_status_record(...) over exposing the raw global.
For this codebase, the best practical option is:
Keep acs_test_status_counters_t in the header.
Move the actual g_rule_test_stats storage into a dedicated implementation file.
Replace direct field access with reset/get/record helpers.
Also, separate from the extern question: this commit removed the only visible reset path from acs_test_infra.c (line 148), and I do not see another reset for g_rule_test_stats in the tree. So the counters now appear to persist for the lifetime of the process. That is another reason to wrap this behind an API.
Change-Id: I987440785dc374780f948e39f993c9bd03eac753