-
Notifications
You must be signed in to change notification settings - Fork 607
Tried to fix the issue ( https://github.com/mandiant/capa/issues/1751 ) #2628
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
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Hello @maxprogrammer007, 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!
Summary of Changes
This pull request addresses issue #1751 by updating the test_ida_features.py
script to be compatible with pytest
version 7.4.0. The primary change involves replacing the deprecated yield
-based test cases with standard assert
statements and refactoring the code to align with modern pytest
practices. Additional changes include improved error handling, scoped imports for IDA compatibility, and input file validation to accommodate different IDA versions. The tests are now marked with @pytest.mark.parametrize
instead of using the @nocollect
decorator, and the test runner has been updated to use pytest.main()
directly.
Highlights
- pytest Compatibility: The PR updates the test script to be compatible with
pytest
7.4.0 by removingyield
-based test cases and using standardassert
statements. - Error Handling: Error handling has been refactored using
pytest.raises()
for better exception management during tests. - IDA Compatibility: Imports are now scoped for IDA compatibility, ensuring
capa.features.extractors.ida.extractor
is only imported within functions. - Input Validation: Input file validation is updated to handle different IDA versions'
GetInputFileMD5()
behavior correctly, usingbinascii.hexlify
when necessary. - Test Runner: The test runner has been updated to use
pytest.main()
directly, and tests are now parameterized.
Changelog
- tests/test_ida_features.py
- Replaced
yield
with standardassert
statements for test cases. - Refactored error handling using
pytest.raises()
. - Added
binascii
import for handling different IDA versions' MD5 return types (line 63). - Scoped imports for IDA compatibility (line 91).
- Removed
@nocollect
decorator and replaced with@pytest.mark.parametrize
(lines 95, 114). - Updated input file validation to handle different IDA versions'
GetInputFileMD5()
behavior correctly (lines 81-84). - Replaced custom test runner with
pytest.main([__file__])
(line 140). - Improved exception reporting with traceback information (lines 111, 130).
- Replaced
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
In IDA's realm, where bytes align,
Pytest's upgrade, a task divine.
From yield's old grace to assert's command,
Tests now flourish, across the land.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
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
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.
Code Review
The pull request addresses issue #1751 by updating the test_ida_features.py
script to be compatible with pytest
7.4.0. The changes include removing deprecated yield
-based test cases, refactoring error handling, scoping imports for IDA compatibility, marking test functions with @nocollect
, updating input file validation, and ensuring proper logging and traceback handling. Overall, the changes seem well-structured and address the identified issue effectively.
Summary of Findings
- Redundant
nocollect
decorator and pytest skip marker: The original code had both@nocollect
and@pytest.mark.skip
decorators, which is redundant. The@nocollect
decorator is not a standard pytest feature and is unnecessary when using pytest's built-in skipping mechanisms. This has been removed in the updated code. - Conversion from yield-based tests to pytest parametrization: The conversion from
yield
-based tests to pytest'sparametrize
decorator is a good practice for modernizing the test suite and ensuring compatibility with newer pytest versions. This improves readability and maintainability. - Error handling with pytest.fail: The use of
pytest.fail
for reporting test failures is appropriate and provides better error messages, including tracebacks, which aids in debugging. - Removal of manual test invocation: The removal of the manual test invocation loop and replacement with
pytest.main([__file__])
simplifies the test execution process and leverages pytest's built-in test discovery and execution capabilities.
Merge Readiness
The pull request appears to address the identified issue effectively and modernizes the test suite. I recommend merging it after addressing the medium severity issue regarding the conditional import of capa.features.extractors.ida.extractor
. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
CHANGELOG updated or no update needed, thanks! 😄
@maxprogrammer007 would you mind sharing a screenshot of these tests running and passing within IDA? |
Sure , will share in few minutes.
…On Mon, 17 Mar, 2025, 1:14 pm Willi Ballenthin, ***@***.***> wrote:
@maxprogrammer007 <https://github.com/maxprogrammer007> would you mind
sharing a screenshot of these tests running and passing within IDA?
—
Reply to this email directly, view it on GitHub
<#2628 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQCHPG4CNQH4M3BVLP2UPID2UZ4MRAVCNFSM6AAAAABZER7KDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMRYGQ3DSOBQGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
[image: williballenthin]*williballenthin* left a comment
(mandiant/capa#2628)
<#2628 (comment)>
@maxprogrammer007 <https://github.com/maxprogrammer007> would you mind
sharing a screenshot of these tests running and passing within IDA?
—
Reply to this email directly, view it on GitHub
<#2628 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQCHPG4CNQH4M3BVLP2UPID2UZ4MRAVCNFSM6AAAAABZER7KDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMRYGQ3DSOBQGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Fix IDA Test Script to Run with Pytest 7.4.0
Summary
This PR updates
test_ida_features.py
to be compatible withpytest
7.4.0 by removing the deprecatedyield
-based test cases. The changes ensure the test script runs correctly within IDA without being mistakenly collected bypytest
.Changes Made
yield
with standardassert
statements to align with modernpytest
best practices.pytest.raises()
where necessary.capa.features.extractors.ida.extractor
is only imported within functions.@nocollect
to prevent pytest from collecting them outside IDA.GetInputFileMD5()
behavior correctly.--CAPA_AUTOEXIT=true
functionality, ensuring the script exits IDA automatically when run from the command line.Fixes Issue
Resolves [mandiant/capa#1751](#1751)
Testing
yield
.[x] No CHANGELOG update needed