Skip to content

feat: Move test status to header file.#341

Closed
SapthagiriP wants to merge 1 commit intoARM-software:mainfrom
SapthagiriP:main
Closed

feat: Move test status to header file.#341
SapthagiriP wants to merge 1 commit intoARM-software:mainfrom
SapthagiriP:main

Conversation

@SapthagiriP
Copy link
Copy Markdown
Contributor

  • make the test status to be used in both VAL and PAL layer

Change-Id: I987440785dc374780f948e39f993c9bd03eac753

   - 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
Copy link
Copy Markdown
Contributor

@prashymh prashymh left a comment

Choose a reason for hiding this comment

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

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.

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.

2 participants