Skip to content
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

Create test_mixins.py #119

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Create test_mixins.py #119

wants to merge 10 commits into from

Conversation

khulnasoft-bot
Copy link
Collaborator

@khulnasoft-bot khulnasoft-bot commented Oct 5, 2024

User description

(Please add to the PR name the issue/s that this PR would close if merged by using a Github keyword. Example: <feature name>. Closes #999. If your PR is made by a single commit, please add that clause in the commit too. This is all required to automate the closure of related issues.)

Description

Please include a summary of the change and link to the related issue.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist

  • I have read and understood the rules about how to Contribute to this project
  • The pull request is for the branch develop
  • [*] A new plugin (analyzer, connector, visualizer, playbook, pivot or ingestor) was added or changed, in which case:
    • I strictly followed the documentation "How to create a Plugin"
    • Usage file was updated.
    • Advanced-Usage was updated (in case the plugin provides additional optional configuration).
    • I have dumped the configuration from Django Admin using the dumpplugin command and added it in the project as a data migration. ("How to share a plugin with the community")
    • If a File analyzer was added and it supports a mimetype which is not already supported, you added a sample of that type inside the archive test_files.zip and you added the default tests for that mimetype in test_classes.py.
    • If you created a new analyzer and it is free (does not require any API key), please add it in the FREE_TO_USE_ANALYZERS playbook by following this guide.
    • Check if it could make sense to add that analyzer/connector to other freely available playbooks.
    • I have provided the resulting raw JSON of a finished analysis and a screenshot of the results.
    • If the plugin interacts with an external service, I have created an attribute called precisely url that contains this information. This is required for Health Checks.
    • If the plugin requires mocked testing, _monkeypatch() was used in its class to apply the necessary decorators.
    • I have added that raw JSON sample to the MockUpResponse of the _monkeypatch() method. This serves us to provide a valid sample for testing.
  • If external libraries/packages with restrictive licenses were used, they were added in the Legal Notice section.
  • Linters (Black, Flake, Isort) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.
  • I have added tests for the feature/bug I solved (see tests folder). All the tests (new and old ones) gave 0 errors.
  • If changes were made to an existing model/serializer/view, the docs were updated and regenerated (check CONTRIBUTE.md).
  • If the GUI has been modified:
    • I have a provided a screenshot of the result in the PR.
    • I have created new frontend tests for the new component or updated existing ones.
  • After you had submitted the PR, if DeepSource, Django Doctors or other third-party linters have triggered any alerts during the CI checks, I have solved those alerts.

Important Rules

  • If you miss to compile the Checklist properly, your PR won't be reviewed by the maintainers.
  • Everytime you make changes to the PR and you think the work is done, you should explicitly ask for a review. After being reviewed and received a "change request", you should explicitly ask for a review again once you have made the requested changes.

PR Type

Tests


Description

  • Introduced a new test suite in test_mixins.py to validate the functionality of VirusTotal mixins.
  • Implemented mock responses to simulate interactions with the VirusTotal API.
  • Developed test cases to ensure correct request parameters and URIs are generated for various observable types.
  • Enhanced test coverage for VirusTotalv3Base and VirusTotalv3Analyzer classes.

Changes walkthrough 📝

Relevant files
Tests
test_mixins.py
Add unit tests for VirusTotal mixins in `test_mixins.py` 

tests/api_app/test_mixins.py

  • Added a new test file test_mixins.py.
  • Implemented mock responses for VirusTotal API interactions.
  • Created test cases for VirusTotalv3Base and VirusTotalv3Analyzer
    classes.
  • Verified request parameters and URIs for different observable types.
  • +191/-0 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Summary by Sourcery

    Introduce a new test suite for VirusTotal mixins by adding test_mixins.py, which includes tests for various observable types and their relationships using mock responses.

    Tests:

    • Add a new test file test_mixins.py to test the functionality of VirusTotal mixins, including VirusTotalv3AnalyzerMixin and VirusTotalv3BaseMixin.

    Signed-off-by: KhulnaSoft bot <[email protected]>
    Copy link

    sourcery-ai bot commented Oct 5, 2024

    Reviewer's Guide by Sourcery

    This pull request introduces a new test file test_mixins.py for the ThreatMatrix project. The file contains unit tests for the VirusTotalv3 mixins, specifically testing the functionality of the VirusTotalv3BaseMixin and VirusTotalv3AnalyzerMixin classes. The tests focus on verifying the correct behavior of the _get_requests_params_and_uri method for different observable types and relationships.

    Class diagram for VirusTotalv3 Mixins

    classDiagram
        class VirusTotalv3BaseMixin {
            <<abstract>>
            +PosixPath python_base_path
            +bool update()
            +dict run()
        }
        class VirusTotalv3AnalyzerMixin {
            <<abstract>>
            +PosixPath python_base_path
            +bool update()
            +dict run()
        }
        class VirusTotalv3Base {
            +PosixPath python_base_path
            +bool update()
            +dict run()
        }
        class VirusTotalv3Analyzer {
            +PosixPath python_base_path
            +bool update()
            +dict run()
        }
        VirusTotalv3Base --|> VirusTotalv3BaseMixin
        VirusTotalv3Analyzer --|> VirusTotalv3AnalyzerMixin
        class ObservableTypes {
            <<enumeration>>
            DOMAIN
            IP
            URL
            HASH
        }
        class AnalyzerConfig {
            +name: str
        }
        class IngestorConfig {
            +name: str
        }
        VirusTotalv3Base o-- IngestorConfig
        VirusTotalv3Analyzer o-- AnalyzerConfig
    
    Loading

    File-Level Changes

    Change Details Files
    Implementation of test classes for VirusTotalv3 mixins
    • Created mock classes VirusTotalv3Base and VirusTotalv3Analyzer
    • Implemented setUp method to initialize test objects
    • Added mock responses for API calls
    tests/api_app/test_mixins.py
    Implementation of test cases for _get_requests_params_and_uri method
    • Added test cases for different observable types (DOMAIN, IP, URL, HASH)
    • Verified correct generation of URI and parameters for each observable type
    • Checked expected relationships for each observable type
    tests/api_app/test_mixins.py

    Tips and commands

    Interacting with Sourcery

    • Trigger a new review: Comment @sourcery-ai review on the pull request.
    • Continue discussions: Reply directly to Sourcery's review comments.
    • Generate a GitHub issue from a review comment: Ask Sourcery to create an
      issue from a review comment by replying to it.
    • Generate a pull request title: Write @sourcery-ai anywhere in the pull
      request title to generate a title at any time.
    • Generate a pull request summary: Write @sourcery-ai summary anywhere in
      the pull request body to generate a PR summary at any time. You can also use
      this command to specify where the summary should be inserted.

    Customizing Your Experience

    Access your dashboard to:

    • Enable or disable review features such as the Sourcery-generated pull request
      summary, the reviewer's guide, and others.
    • Change the review language.
    • Add, remove or edit custom review instructions.
    • Adjust other review settings.

    Getting Help

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Hardcoded API key:
    The test file contains a hardcoded API key value (line 108). Although this is a test file, it's generally not recommended to include actual API keys in the code, even for testing purposes. Consider using environment variables or a secure configuration management system for handling API keys, even in test environments.

    ⚡ Recommended focus areas for review

    Hardcoded API Key
    The test file contains a hardcoded API key value, which is not a good practice for security reasons.

    Incomplete Test Coverage
    The test case only covers the _get_requests_params_and_uri method, but other methods of the mixins are not tested.

    Copy link

    deepsource-io bot commented Oct 5, 2024

    Here's the code health analysis summary for commits 6289944..a5813b5. View details on DeepSource ↗.

    Analysis Summary

    AnalyzerStatusSummaryLink
    DeepSource Python LogoPython❌ Failure
    ❗ 39 occurences introduced
    🎯 25 occurences resolved
    View Check ↗
    DeepSource Docker LogoDocker✅ SuccessView Check ↗

    💡 If you’re a repository administrator, you can configure the quality gates from the settings.

    Copy link

    @sourcery-ai sourcery-ai bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    We have skipped reviewing this pull request. It seems to have been created by a bot (hey, khulnasoft-bot!). We assume it knows what it's doing!

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use parameterized tests to reduce code duplication and improve maintainability

    Instead of repeating the test structure for different observable types, consider
    using a parameterized test. This will make the test more maintainable and easier to
    extend for new observable types in the future.

    tests/api_app/test_mixins.py [110-143]

    -def test_get_requests_params_and_uri(self):
    -    expected_relationships = [
    -        "communicating_files",
    -        "historical_whois",
    -        "referrer_files",
    -        "resolutions",
    -        "siblings",
    -        "subdomains",
    -        "collections",
    -        "historical_ssl_certificates",
    -    ]
    +import unittest
    +
    +@unittest.parameterize("observable_type, value, expected_relationships, expected_uri", [
    +    (ObservableTypes.DOMAIN, "google.com", 
    +     ["communicating_files", "historical_whois", "referrer_files", "resolutions", 
    +      "siblings", "subdomains", "collections", "historical_ssl_certificates"],
    +     "domains/google.com"),
    +    (ObservableTypes.IP, "8.8.8.8", 
    +     ["communicating_files", "historical_whois", "referrer_files", "resolutions", 
    +      "collections", "historical_ssl_certificates"],
    +     "ip_addresses/8.8.8.8"),
    +    # Add more test cases for other observable types
    +])
    +def test_get_requests_params_and_uri(self, observable_type, value, expected_relationships, expected_uri):
         params, uri, relationships_requested = self.base._get_requests_params_and_uri(
    -        ObservableTypes.DOMAIN, "google.com", True
    +        observable_type, value, True
         )
         self.assertIn("relationships", params)
         self.assertListEqual(relationships_requested, expected_relationships)
         self.assertEqual(params["relationships"], ",".join(expected_relationships))
    -    self.assertEqual(uri, "domains/google.com")
    +    self.assertEqual(uri, expected_uri)
     
    -    expected_relationships = [
    -        "communicating_files",
    -        "historical_whois",
    -        "referrer_files",
    -        "resolutions",
    -        "collections",
    -        "historical_ssl_certificates",
    -    ]
    -    params, uri, relationships_requested = self.base._get_requests_params_and_uri(
    -        ObservableTypes.IP, "8.8.8.8", True
    -    )
    -    # ... (similar assertions)
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Parameterizing the test reduces code duplication and enhances maintainability, making it easier to extend the test for new observable types. This is a significant improvement in terms of test organization and scalability.

    9
    Maintainability
    Remove duplicate assignment of the API key name

    The setUp method is assigning the same value to self._api_key_name twice. Remove the
    duplicate assignment to improve code clarity.

    tests/api_app/test_mixins.py [108]

    -self._api_key_name = self._api_key_name = "123456"
    +self._api_key_name = "123456"
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Removing the duplicate assignment improves code clarity and maintainability. It eliminates unnecessary redundancy, making the code cleaner and easier to understand.

    8
    Enhancement
    Use a more specific assertion method for checking dictionary contents

    Consider using a more specific assertion method like assertDictContainsSubset
    instead of assertIn to check if the 'relationships' key is in the params dictionary
    and its value matches the expected relationships. This will provide a more precise
    test and better error messages if the test fails.

    tests/api_app/test_mixins.py [124-126]

    -self.assertIn("relationships", params)
    +self.assertDictContainsSubset({"relationships": ",".join(expected_relationships)}, params)
     self.assertListEqual(relationships_requested, expected_relationships)
    -self.assertEqual(params["relationships"], ",".join(expected_relationships))
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use assertDictContainsSubset provides a more precise test and better error messages, improving test reliability and debugging. However, assertDictContainsSubset is deprecated in Python 3.2 and later, so this suggestion may not be applicable depending on the Python version used.

    7
    Possible issue
    Add tests for the update method in both classes

    Consider adding assertions to verify that the update method is called for both
    VirusTotalv3Base and VirusTotalv3Analyzer classes. This will ensure that the update
    method is properly implemented and called, even though it currently does nothing.

    tests/api_app/test_mixins.py [72-76]

     @classmethod
     def update(cls) -> bool:
    -    pass
    +    return True
     
     def run(self) -> dict:
         return {}
     
    +# In the test method:
    +def test_update_method(self):
    +    self.assertTrue(VirusTotalv3Base.update())
    +    self.assertTrue(VirusTotalv3Analyzer.update())
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding assertions for the update method ensures that it is implemented and called correctly. However, since the method currently does nothing, the impact of this suggestion is limited unless the method's functionality is expanded in the future.

    6

    💡 Need additional feedback ? start a PR chat

    Signed-off-by: KhulnaSoft bot <[email protected]>
    Signed-off-by: KhulnaSoft bot <[email protected]>
    Signed-off-by: KhulnaSoft bot <[email protected]>
    Signed-off-by: KhulnaSoft bot <[email protected]>
    Signed-off-by: KhulnaSoft bot <[email protected]>
    Signed-off-by: KhulnaSoft bot <[email protected]>
    @khulnasoft-bot khulnasoft-bot changed the base branch from master to develop October 5, 2024 19:53
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant