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

Fix teams #551

Merged
merged 3 commits into from
Jan 1, 2025
Merged

Fix teams #551

merged 3 commits into from
Jan 1, 2025

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Jan 1, 2025

PR Type

Bug fix, Enhancement


Description

  • Implement Teams API pagination for complete message retrieval

  • Optimize message assertion logic and conditions

  • Add message count logging for better debugging

  • Remove redundant logging in misconfiguration checks


Changes walkthrough 📝

Relevant files
Enhancement
teams_workflows.py
Optimize Teams message assertion logic and logging             

tests_scripts/workflows/teams_workflows.py

  • Simplified message assertion logic by combining conditions
  • Added logging for number of messages found in assertions
  • Removed redundant debug logging in misconfiguration checks
  • +2/-9     
    Bug fix
    utils.py
    Add pagination support for Teams message retrieval             

    tests_scripts/workflows/utils.py

  • Implemented pagination for Teams API message fetching
  • Added support for handling @odata.nextLink responses
  • Enhanced message collection to gather all available pages
  • +9/-2     

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

    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

    Assertion Logic

    The assertion message states "exactly one" but the assertion checks for greater than 0 messages. This inconsistency should be validated.

    assert found > 0, f"expected to have exactly one new misconfiguration message, found {found}"
    Error Handling

    The Teams API request lacks error handling for failed requests or invalid responses. Should handle potential HTTP errors and JSON parsing failures.

    response = requests.get(endpoint, headers=headers)
    data = response.json()
    messages.extend(data.get('value', []))

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix inconsistency between assertion message and actual validation logic

    The assertion message states "exactly one" but the code checks for greater than
    zero, which could allow multiple messages when only one is expected.

    tests_scripts/workflows/teams_workflows.py [203]

    -assert found > 0, f"expected to have exactly one new misconfiguration message, found {found}"
    +assert found == 1, f"expected to have exactly one new misconfiguration message, found {found}"
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The assertion message claims to expect exactly one message but the code allows more than one, which could mask bugs. This mismatch between code behavior and error message could lead to test reliability issues.

    9
    Add error handling for HTTP requests to prevent silent failures and improve reliability

    Add error handling for the Teams API request to handle potential network issues or
    API errors. The current implementation assumes the request will always succeed.

    tests_scripts/workflows/utils.py [88-90]

     response = requests.get(endpoint, headers=headers)
    +response.raise_for_status()  # Raises an HTTPError for bad responses
     data = response.json()
     messages.extend(data.get('value', []))
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling with raise_for_status() is critical for detecting API failures early rather than silently continuing with invalid data. This is especially important in test code to avoid false positives.

    8
    General
    Add request timeout to prevent test hanging on slow or unresponsive API

    Add a timeout parameter to the requests.get() call to prevent the test from hanging
    indefinitely if the Teams API is unresponsive.

    tests_scripts/workflows/utils.py [88]

    -response = requests.get(endpoint, headers=headers)
    +response = requests.get(endpoint, headers=headers, timeout=30)  # 30 seconds timeout
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a timeout is important for test reliability to prevent indefinite hangs on network issues. This is a good defensive programming practice for network calls in test code.

    7

    Copy link

    github-actions bot commented Jan 1, 2025

    Failed to generate code suggestions for PR

    @kooomix kooomix merged commit 4c1fde9 into master Jan 1, 2025
    2 checks passed
    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