Conversation
…t and mram_test; added & fixed bugs in docs
There was a problem hiding this comment.
Pull request overview
This PR standardizes the repository’s C testing approach by introducing a shared test harness and migrating existing filesys_test/mram_test code away from the prior hardware-specific assert override.
Changes:
- Added
src/test_infrastructure/test_harness.{h,c}withTEST_ASSERTand suite runners (run/include/exclude). - Removed the deprecated
hardware_test_assertmechanism and updated Bazel macros/deps to auto-link the new infrastructure. - Refactored
filesys_testandmram_testto useTEST_ASSERT/ harness patterns and updatedTESTING.mddocumentation accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test_infrastructure/test_harness.h | Defines TEST_ASSERT, test case struct, and harness runner APIs. |
| src/test_infrastructure/test_harness.c | Implements suite running plus include/exclude selection logic. |
| src/test_infrastructure/hardware_test_assert.h | Removed legacy non-fatal ASSERT override for bringup tests. |
| src/test_infrastructure/BUILD.bazel | Wires harness into hardware_test_infrastructure and test_infrastructure targets. |
| src/filesys/test/mram_test.h | Migrates MRAM tests to harness style (int return + TEST_ASSERT). |
| src/filesys/test/mram_test.c | Updates MRAM test functions to return int and use TEST_ASSERT. |
| src/filesys/test/filesys_test.h | Exposes a test-case table (filesys_tests) for harness execution. |
| src/filesys/test/filesys_test.c | Replaces ad-hoc runner with a test_harness_case_t table + harness invocation. |
| src/filesys/test/filesys_integration_test.c | Runs the shared filesys_tests suite via test_harness_run. |
| bzl/defs.bzl | Updates Bazel macros to depend on explicit :test_infrastructure target; ensures deps are mutable for integration macro. |
| TESTING.md | Documents the new harness workflow and updates integration entrypoint guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR standardizes the project’s test execution by introducing a shared C test harness and updating existing filesystem/MRAM tests and build macros/docs to use the new infrastructure across unit and hardware integration test entry points.
Changes:
- Added
test_harnessAPI +TEST_ASSERTmacro and wired it into Bazel test macros (samwise_test/samwise_integration_test). - Refactored
filesys_testto expose a test-case table and run viatest_harness_run(also reused by the integration entrypoint). - Updated documentation in
TESTING.mdto describe the new harness approach and removed the oldhardware_test_assertoverride.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test_infrastructure/test_harness.h | Defines TEST_ASSERT, test case struct, and harness function prototypes |
| src/test_infrastructure/test_harness.c | Implements suite runner + include/exclude selection helpers |
| src/test_infrastructure/hardware_test_assert.h | Removed old non-fatal ASSERT override |
| src/test_infrastructure/BUILD.bazel | Updates infrastructure targets to ship the harness |
| src/filesys/test/mram_test.h | Converts test APIs to int return and adopts harness include |
| src/filesys/test/mram_test.c | Converts assertions to TEST_ASSERT and changes tests to return int |
| src/filesys/test/filesys_test.h | Exports filesys_tests[] and setup API for harness use |
| src/filesys/test/filesys_test.c | Replaces custom runner with harness-backed test table + main() |
| src/filesys/test/filesys_integration_test.c | Uses harness to run the same exported test table on hardware |
| bzl/defs.bzl | Updates default test deps to point at the correct test_infrastructure target |
| TESTING.md | Documents harness usage and updates integration test entrypoint guidance |
Comments suppressed due to low confidence (1)
src/filesys/test/mram_test.c:201
- These test functions now return
intand useTEST_ASSERT(which returns-1on failure), butmain()ignores the return codes and always logs "All MRAM tests passed!" and returns 0. This will report success even when tests fail. Aggregate/propagate failures (or usetest_harness_run()here) so the program exits with non-zero when any test fails.
int main()
{
LOG_DEBUG("Starting MRAM test\n");
mram_init();
test_mram_write_read();
test_mram_write_disable_enable();
test_mram_preserve_data_on_sleep();
// test_mram_check_collision();
// test_mram_clear();
test_mram_read_status();
LOG_DEBUG("All MRAM tests passed!\n");
// LOG_DEBUG("Running allocation tests...\n");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
devYaoYH
left a comment
There was a problem hiding this comment.
Largely LGTM apart from minor changes to the behavior of custom_init_func to be clarified in comments.
This change adds a test runner helper function that wraps a list of test functions for consistent testing interface across filesys and mram hardware tests.
Also changes ASSERT --> TEST_ASSERT to better differentiate their respective use-cases. The behavior of the latter is a macro that logs with "[ERROR]" and returns -1 (outer while loop is a robust macro wrapping pattern).
devYaoYH
left a comment
There was a problem hiding this comment.
LGTM, thanks for the changes! Agreed, we. an perform the bazel default deps migration in a separate change.
I also tested to make sure the code still runs on the physical hardware, and it works. (However, the actual tests are still certainly failing, because of #268).
Closes #284