fixtures: add dynamic sample discovery and reverse MD5 lookup #2831
Conversation
Summary of ChangesHello @kami922, 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 introduces a significant improvement to how test fixtures are managed by transitioning from a brittle, manually updated system to a dynamic, automated discovery and indexing mechanism. By building a comprehensive cache of test samples at module import, it enables efficient lookups by various identifiers, including a new reverse MD5-to-name lookup. This enhancement not only streamlines the process of adding new test data but also boosts the reliability and scalability of the testing infrastructure. 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. 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. Footnotes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic sample discovery and a reverse MD5 lookup function, significantly improving the maintainability and scalability of test fixtures. The new _build_sample_cache function efficiently indexes samples by various attributes, and get_sample_short_name_by_md5 provides a convenient way to retrieve sample names from their MD5 hashes. Comprehensive tests have been added to validate the new functionality, including forward, reverse, and round-trip lookups. Overall, this is a well-implemented solution that addresses the maintenance burden of hardcoded lists.
| cache[md5.upper()] = path | ||
|
|
||
| # Index by filename | ||
| cache[path.name] = path |
There was a problem hiding this comment.
The cache uses path.name as a key for indexing by filename. If there are multiple files with the same name but in different subdirectories (e.g., data/subdir1/file.txt and data/subdir2/file.txt), only the last encountered file will be stored in the cache for that path.name. This could lead to unexpected behavior if a specific file is expected when looking up by filename. Consider if path.name is guaranteed to be unique across all samples, or if a more robust key (e.g., relative path) is needed for filename lookups to prevent collisions.
tests/fixtures.py
Outdated
| raise ValueError(f"unexpected sample fixture: {name}") | ||
|
|
||
|
|
||
| def get_sample_short_name_by_md5(md5): |
There was a problem hiding this comment.
Please add a type hint for the md5 parameter to improve code readability and enable static analysis. Based on the docstring, str would be appropriate.
| def get_sample_short_name_by_md5(md5): | |
| def get_sample_short_name_by_md5(md5: str): |
tests/test_fixtures.py
Outdated
| """Test reverse MD5 lookup returns correct sample name.""" | ||
| result = get_sample_short_name_by_md5(md5) | ||
| # The result should match the expected name (or be a prefix for hash-named files) | ||
| assert name in result or result in name |
There was a problem hiding this comment.
For SIMPLE_SAMPLES, the get_sample_short_name_by_md5 function should return the exact name (e.g., "mimikatz"). The current assertion assert name in result or result in name is too broad and might pass even if the stem extraction is not perfectly accurate (e.g., if it returns "mimikatz.exe"). Please change this to an exact equality check.
| assert name in result or result in name | |
| assert result == name |
tests/test_fixtures.py
Outdated
| md5 = get_sample_md5_by_name(name) | ||
| result_name = get_sample_short_name_by_md5(md5) | ||
| # For simple names, result should contain the original name | ||
| assert name in result_name or result_name in name |
There was a problem hiding this comment.
Similar to test_reverse_lookup, for SIMPLE_SAMPLES, the round-trip lookup should result in an exact match for the name. The current assertion assert name in result_name or result_name in name is too lenient. Please update it to assert result_name == name for precise validation.
| assert name in result_name or result_name in name | |
| assert result_name == name |
github-actions
bot
dismissed
their stale review
CHANGELOG updated or no update needed, thanks! 😄
|
@mike-hunhoff @mr-tz Hello I am working on CI failed tests meanwhile can you please tell me is running CI tests locally Via "act" a good approach? so I can see failed tests and fix them before committing and pushing. currently I am not following this approach if yes could you also tell me few guidelines and best practices related to this project. |
|
using "act" is fine. sometimes i do this to avoid waiting for the round trip of committing, pushing, and waiting for GH Actions to spawn and complete. i'm not quite sure what hints would help. i'd suggest running the linters, formatters, and tests before submitting a PR. what else? |
ok alot of tests are failing currently I will fix them. but one confusion I am having currently regarding running act locally is that I think act might take a lot of disk space of my laptop even tho docker image is pulled and current failing tests might according to my comprehension are related to OS. anyways I will experiment with it then fix it and commit again |
Walk the tests/data directory at module import time to build a cache indexed by MD5, SHA256, filename, and stem. Add get_sample_short_name_by_md5() for reverse lookups, eliminating the need for hardcoded hash mappings. Closes mandiant#1743
kami922
force-pushed
the
fix-1743-dynamic-sample-discovery
branch
from
d9b662c to
98227bf
Compare
2 participants
Summary
Implements dynamic file discovery for test fixtures, addressing issue #1743.
This is a cleaned-up resubmission of #2802, incorporating maintainer feedback.
Background
Issue #1743 requested adding
get_sample_short_name_by_md5()as the inverse of the existingget_sample_md5_by_name()function. The previous PR #2802 was closed with feedback to follow the original issue's vision of using dynamic discovery instead of maintaining hardcoded lists.Problem
The codebase maintains hardcoded lists of file names and MD5 hashes in
get_sample_md5_by_name(), requiring manual updates whenever test samples are added. This creates maintenance burden and potential for errors.Solution
This PR implements dynamic sample discovery as requested by @williballenthin in #1743:
_build_sample_cache()- Walkstests/data/directory once at module import timeget_sample_short_name_by_md5()- Reverse MD5→name lookup using the dynamic cacheBenefits
Implementation Details
Follows the approach suggested by @williballenthin in #1743:
Implementation is similar to
collect_samples()inscripts/lint.py, as referenced in the issue discussion.Changes
tests/fixtures.py: Add cache building and reverse lookup function (+90 lines)tests/test_fixtures.py: Add parametrized tests for lookups and roundtrips (+126 lines)Testing