test: add comprehensive tests for API extraction helpers - #2942
test: add comprehensive tests for API extraction helpers#2942231B145-JUET wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased) section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the test suite for the API extraction helper functions within Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request does an excellent job of adding comprehensive tests for the API extraction helper functions. The new tests are well-structured into classes and cover many edge cases. I have a couple of suggestions to make the test suite even more robust, particularly for reformat_forwarded_export_name and carve_pe.
| def test_api_ms_dll(self): | ||
| """DLL names with dashes and multiple segments should be lowercased correctly.""" | ||
| result = helpers.reformat_forwarded_export_name( | ||
| "api-ms-win-core-synch-l1-1-0.SleepEx" |
There was a problem hiding this comment.
The current test for api-ms-dll names uses an already lowercase DLL name. To properly test that the function correctly lowercases DLL names with dashes, it would be better to use an uppercase DLL name in the test case. This will make the test more robust.
| "api-ms-win-core-synch-l1-1-0.SleepEx" | |
| "API-MS-WIN-CORE-SYNCH-L1-1-0.SleepEx" |
| class TestCarvePe: | ||
| """test the carve_pe helper for finding embedded PE files.""" | ||
|
|
||
| def test_empty_buffer(self): | ||
| """Empty buffer should yield no results.""" | ||
| assert list(helpers.carve_pe(b"")) == [] | ||
|
|
||
| def test_no_pe(self): | ||
| """Buffer without MZ header should yield no results.""" | ||
| assert list(helpers.carve_pe(b"\x00" * 1024)) == [] | ||
|
|
||
| def test_truncated_mz(self): | ||
| """Buffer with MZ but too short for e_lfanew should yield no results.""" | ||
| assert list(helpers.carve_pe(b"MZ" + b"\x00" * 10)) == [] |
There was a problem hiding this comment.
The test suite for carve_pe currently only includes negative test cases (e.g., empty buffer, no PE header). To ensure the function works correctly, it's important to add positive test cases that verify it can successfully identify embedded PE files.
Consider adding tests for:
- A buffer containing a simple, valid PE file.
- A buffer containing a XOR-encoded PE file.
This would make the test coverage for this complex function truly "comprehensive" as intended by the PR.
williballenthin
left a comment
There was a problem hiding this comment.
this code style is not consistent with the rest of the project and the code obviously AI generated. while the latter doesn't bother me, the former does, because it doesn't demonstrate much effort on behalf of the author. closing the PR to avoid distracting other maintainers from their work.
Resolves #1899.
Added comprehensive test coverage for all helper functions in
capa/features/extractors/helpers.py, including:is_aw_functionedge casesis_ordinaledge casesgenerate_symbolshandling of.dll,.drv,.so, case normalization, and empty DLL namesreformat_forwarded_export_namexor_staticandtwos_complementcarve_peThis provides the requested test coverage for API pattern variants before refactoring.