-
Notifications
You must be signed in to change notification settings - Fork 274
Update API tests to use native helper #4881
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: main
Are you sure you want to change the base?
Update API tests to use native helper #4881
Conversation
shankarseal
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.
I would like the PR to take the following changes:
ebpf_native_load_drivermust usentstatus_to_ebpf_resultfrom the status returned byZwLoadDriverinstead of returning a hard codedEBPF_FAILEDand return using theEBPF_RETURNmacro.ebpf_native_loadmust log the error code returned byebpf_native_load_driver.
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.
Pull request overview
This PR refactors API test helper functions to address timing issues when multiple tests use the same native module by moving native_module_helper_t logic into the program_load_helper() function. This eliminates the need for individual test cases to explicitly manage native module copies.
Key Changes:
- Integrated
native_module_helper_tfunctionality directly intoprogram_load_helper()with a new optionalcopy_fileparameter - Added helper functions
_is_native_program()and_get_file_name_without_extension()to detect and process native driver files - Simplified multiple test cases by removing explicit
native_module_helper_tinstantiations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/api_test/api_test.h | Added helper functions for native file detection and path processing; integrated native module copying logic into program_load_helper() with new copy_file parameter (defaults to true) |
| tests/api_test/api_test.cpp | Removed explicit native_module_helper_t usage from multiple test cases; updated tests to pass file names directly to helper functions; added copy_file=false parameter for special test cases that require loading the original file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _is_native_program(_In_z_ const char* file_name) | ||
| { | ||
| std::string file_name_string(file_name); | ||
| std::string file_extension = file_name_string.substr(file_name_string.find_last_of(".") + 1); |
Copilot
AI
Jan 7, 2026
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.
The function _is_native_program() has a bug when the filename doesn't contain a dot. If find_last_of(".") returns std::string::npos (which is typically SIZE_MAX), then substr(std::string::npos + 1) will wrap around to 0 and return the entire string, or could cause undefined behavior. You should check if find_last_of returns std::string::npos before calling substr.
| std::string file_extension = file_name_string.substr(file_name_string.find_last_of(".") + 1); | |
| size_t last_dot = file_name_string.find_last_of('.'); | |
| std::string file_extension; | |
| if (last_dot != std::string::npos && last_dot + 1 < file_name_string.size()) { | |
| file_extension = file_name_string.substr(last_dot + 1); | |
| } |
|
|
||
| if (copy_file && _is_native_program(file_name)) { | ||
| std::string file_name_without_extension = _get_file_name_without_extension(file_name); | ||
| _native_helper.initialize(file_name_without_extension.c_str(), execution_type); |
Copilot
AI
Jan 7, 2026
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.
When a native program file (.sys) is detected, the native_module_helper should be initialized with EBPF_EXECUTION_NATIVE instead of the passed execution_type. If execution_type is EBPF_EXECUTION_ANY, in non-JIT-disabled builds it won't resolve to EBPF_EXECUTION_NATIVE, causing the helper to incorrectly try to use a .o file instead of creating a copy of the .sys file. This breaks tests like test_ebpf_program_next_previous_native that pass .sys files but rely on program_load_helper being called with EBPF_EXECUTION_ANY. The initialization should use EBPF_EXECUTION_NATIVE when a .sys file is detected.
| _native_helper.initialize(file_name_without_extension.c_str(), execution_type); | |
| _native_helper.initialize(file_name_without_extension.c_str(), EBPF_EXECUTION_NATIVE); |
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.
I disagree with copilot - if invalid test parameters are used the test (or call within the test) should fail rather than being hidden within the helper. This comment looks like it points to a bug in the helper though.
@shankarseal since this is a test only PR, is it fine to make the above 2 changes in a separate PR? I can file an issue to track the above 2 items. |
Description
If two tests are using the same native module, there is a possible timing issue that the second test fails as the driver from first test case is not yet unloaded. There is already a test helper class
native_module_helper_tthat creates a copy of the native driver to enable tests to load it multiple times, but in some tests it is missed to use this helper, and the original driver is directly loaded.This PR updates
program_load_helper()helper in API tests to internally usenative_module_helper_tso that each caller does not have to remember to do the same.This issue was causing random failures in api_tests for ARM64 builds.
Testing
Existing CICD
Documentation
No
Installation
No