-
Notifications
You must be signed in to change notification settings - Fork 11
Add timed test suite wrapper macros for process_watchdog integration #520
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
base: master
Are you sure you want to change the base?
Conversation
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
/AzurePipelines run #Resolved |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/AzurePipelines run #Resolved |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
/AzurePipelines run #Resolved |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/AzurePipelines run #Resolved |
|
Azure Pipelines successfully started running 1 pipeline(s). |
common/tests/CMakeLists.txt
Outdated
| build_test_folder(thandle_ptr_ut) | ||
| build_test_folder(real_thandle_log_context_handle_ut) | ||
| build_test_folder(tqueue_ut) | ||
| add_subdirectory(timed_test_suite_ut) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use c-testrunnerswitcher.
| if (timed_test_suite_ut_succeeded() != 0) | ||
| { | ||
| // Final validation of fixture ordering has failed | ||
| failedTests++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we Log something here? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added log
| add_subdirectory(timed_test_suite_ut) | ||
| endif() | ||
|
|
||
| if(${run_int_tests}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider an int test that demonstrates the timeout works. I guess since the process_watchdog will terminate, the test would probably need to launch that process and make sure it gets terminated within the timeout (+ delta)
mattdurak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
| static int call_counter = 0; | ||
|
|
||
| // Mock process_watchdog functions | ||
| int process_watchdog_init(uint32_t timeout_ms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called inside TIMED_TEST_SUITE_INITIALIZE macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added specs and comment.
| return 0; | ||
| } | ||
|
|
||
| void process_watchdog_deinit(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called inside TIMED_TEST_SUITE_CLEANUP macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added specs and comment
| // This ensures the macros use our mock functions | ||
| #include "c_pal/timed_test_suite.h" | ||
|
|
||
| CTEST_BEGIN_TEST_SUITE(timed_test_suite_ut) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIMED_TEST_SUITE_INITIALIZE/TIMED_TEST_SUITE_CLEANUP are the macros under test.
CTEST_BEGIN_TEST_SUITE/CTEST_END_TEST_SUITE are being used as the testing framework.
How else can we write these tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use BEGIN_TEST_SUITE/END_TEST_SUITE.
Does that make it better?
| // Verify watchdog deinit was called AFTER user cleanup | ||
| if (watchdog_deinit_call_order <= user_cleanup_call_order) | ||
| { | ||
| (void)printf("ERROR: watchdog_deinit_call_order (%d) should be greater than user_cleanup_call_order (%d)\r\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing to use LogError.
| { | ||
| size_t failedTests = 0; | ||
|
|
||
| (void)logger_init(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added check
| } | ||
| else | ||
| { | ||
| result = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be validated in the test suite cleanup and then we do not need the shoehorn-y timed_test_suite_ut_succeeded to be called from main.
I think it would make the test cleaner. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dcristoloveanu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
- Switch from CTEST_* macros to testrunnerswitcher macros for consistency - Add LogError when fixture ordering validation fails - Add detailed comment explaining mock function usage pattern - Convert from add_subdirectory to build_test_folder/build_test_artifacts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace printf with LogError in timed_test_suite_ut_succeeded() - Handle logger_init() failure properly in main.c - Only run tests if logger initialization succeeds Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove SRS_TIMED_TEST_SUITE_43_001 (default timeout constant) from requirements, code tags, and test tags - Add timeout value assertion in process_watchdog_init mock to verify the correct timeout_ms is passed - Add additional fixtures (additional_init_fixture, additional_cleanup_fixture) to verify variadic argument passing and fixture ordering - Add comprehensive fixture ordering assertions throughout Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
|
||
| if (logger_init() != 0) | ||
| { | ||
| (void)printf("logger_init failed\r\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Copyright (C) Microsoft Corporation. All rights reserved. | ||
|
|
||
| #include <stdint.h> | ||
| #include <inttypes.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only need inttypes, dont need stdint also
| // including timed_test_suite.h, we can track when they are called to verify fixture ordering. | ||
| // Expected timeout value (must match TIMED_TEST_DEFAULT_TIMEOUT_MS from timed_test_suite.h) | ||
| #define EXPECTED_TIMEOUT_MS 600000 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the above the comment explaining the mock process_watchdog functions
|
No pipelines are associated with this pull request. |
3 similar comments
|
No pipelines are associated with this pull request. |
|
No pipelines are associated with this pull request. |
|
No pipelines are associated with this pull request. |
| "user_init should be call #3"); | ||
| } | ||
|
|
||
| /*Tests_SRS_TIMED_TEST_SUITE_43_006: [ TIMED_TEST_SUITE_CLEANUP shall call TEST_SUITE_CLEANUP with any additional fixtures passed via variadic arguments, followed by the watchdog deinit fixture as the last fixture. ]*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #define EXPECTED_TIMEOUT_MS 600000 | ||
|
|
||
| /*Tests_SRS_TIMED_TEST_SUITE_43_002: [ TIMED_TEST_SUITE_INITIALIZE shall create a static fixture function that calls process_watchdog_init with timeout_ms. ]*/ | ||
| int process_watchdog_init(uint32_t timeout_ms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…leanup includes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
TIMED_TEST_SUITE_INITIALIZEandTIMED_TEST_SUITE_CLEANUPmacros incommon/inc/c_pal/timed_test_suite.hprocess_watchdogDependencies