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

Update system test mapping and enhance attack chain assertions in sce… #583

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Jan 16, 2025

User description

…narios manager


PR Type

Enhancement, Tests


Description

  • Enhanced attack chain validation in scenarios_manager.py.

  • Improved assertion logic for attack chain results.

  • Updated system test mapping with dummy repository names.

  • Refactored expected attack chain node extraction logic.


Changes walkthrough 📝

Relevant files
Enhancement
scenarios_manager.py
Enhance attack chain validation logic and assertions         

systest_utils/scenarios_manager.py

  • Added assertion to check for 'response' key in results.
  • Refactored expected attack chain node extraction logic.
  • Improved attack chain validation logic with better assertions.
  • Ensured loop breaks upon finding the expected attack chain.
  • +5/-2     
    Tests
    system_test_mapping.json
    Update system test mapping with dummy repository names     

    system_test_mapping.json

  • Updated target repository names to include '-dummy' suffix.
  • No functional changes to test descriptions or ownership.
  • +4/-4     

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • @kooomix kooomix merged commit efaf7b2 into master Jan 16, 2025
    3 checks passed
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logic Flow

    The break statement on line 235 exits the loop after finding the first matching attack chain. Verify this is the intended behavior and won't miss validating other relevant attack chains.

    break
    Error Handling

    The new assertion for 'response' key should include a more descriptive error message to help debug failures.

    assert "response" in result, f"'response' key not found in the result"

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add defensive programming to handle missing dictionary keys that could cause runtime errors

    Add error handling for accessing the 'attackChainNodes' key in case it's missing
    from the response structure. Currently the code assumes this key exists which could
    cause KeyError exceptions.

    systest_utils/scenarios_manager.py [228-229]

    -ac_node_result = result['response']['attackChains'][acid]['attackChainNodes']
    -ac_node_expected = expected_ac['attackChainNodes']
    +ac_node_result = result['response']['attackChains'][acid].get('attackChainNodes')
    +ac_node_expected = expected_ac.get('attackChainNodes')
    +assert ac_node_result is not None, "Missing 'attackChainNodes' in result"
    +assert ac_node_expected is not None, "Missing 'attackChainNodes' in expected data"
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds important error handling for dictionary key access that could prevent cryptic KeyError exceptions and provide more meaningful error messages. This is particularly valuable in test code where clear error messages are crucial.

    8

    Copy link

    Failed to generate code suggestions for PR

    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