Skip to content

Feature-339: Accept --max-run-time-sec in test_with_docker.sh #340

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.

Sign up for GitHub

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

Jump to bottom
Open
andsimakov wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Feature-339: Accept --max-run-time-sec in test_with_docker.sh #340

andsimakov wants to merge 3 commits into from

Conversation

@andsimakov
Copy link
Contributor

@andsimakov andsimakov commented May 27, 2025 •
edited
Loading

PR Type

Enhancement

Description

  • Added --max-run-time-sec argument support to test_with_docker.sh

  • Passed --max-run-time-sec from integration test script

  • Updated command construction to include max run time

Changes walkthrough 📝

Relevant files
Enhancement
test_with_docker.sh
Add and handle --max-run-time-sec argument in test_with_docker.sh

tests_integration/test_with_docker.sh

  • Introduced MAX_RUN_TIME_SEC variable with default value
  • Parsed --max-run-time-sec argument in CLI options
  • Appended --max-run-time-sec to agent command if set
    		• +6/-0     
    test_all.sh
    Pass max run time argument to test_with_docker.sh               

    tests_integration/test_all.sh

    • Passed --max-run-time-sec 240 to test_with_docker.sh invocation
    		 +1/-0     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-free-for-open-source-projects
    Copy link

    qodo-free-for-open-source-projects bot commented May 27, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @qodo-free-for-open-source-projects
    Copy link

    qodo-free-for-open-source-projects bot commented May 27, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Fix conditional logic

    The condition checks if MAX_RUN_TIME_SEC is non-empty, but the variable is
    initialized with a default value of 30, so it will always be non-empty. You
    should check if it's non-zero or different from the default value instead.

    tests_integration/test_with_docker.sh [120-122]

    -if [ -n "$MAX_RUN_TIME_SEC" ]; then
+if [ "$MAX_RUN_TIME_SEC" != "30" ]; then
   COMMAND="$COMMAND --max-run-time-sec $MAX_RUN_TIME_SEC"
 fi
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a logical flaw where MAX_RUN_TIME_SEC is always non-empty due to the default value, causing --max-run-time-sec 30 to always be added even when not specified by the user. The proposed fix appropriately only adds the parameter when it differs from the default.

    		Medium
    • Update

    @andsimakov andsimakov changed the title Freature-339: Accept --max-run-time-sec in test_with_docker.sh Feature-339: Accept --max-run-time-sec in test_with_docker.sh May 27, 2025
    andsimakov
    andsimakov commented May 27, 2025
    View reviewed changes
    cover_agent/cover_agent.py
    self.logger.info(
    f"Reached above target coverage of {desired_coverage}% "
    f"(Current Coverage: {current_coverage}%) in {iteration_count} iterations."
    f"(Current Coverage: {current_coverage}%) in {iteration_count + 1} iterations."
    Copy link
    Contributor Author

    @andsimakov andsimakov May 27, 2025 •
    edited
    Loading

    Choose a reason for hiding this comment

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

    This fixes a situation where the desired coverage is reached during the first iteration, but it appears as if it was reached in 0 iterations.

    andsimakov
    andsimakov commented May 27, 2025
    View reviewed changes
    cover_agent/lsp_logic/ContextHelper.py
    from pathlib import Path
    from typing import AsyncIterator, List, Tuple, Optional
    from cover_agent.AICaller import AICaller
    from cover_agent.ai_caller import AICaller
    Copy link
    Contributor Author

    @andsimakov andsimakov May 27, 2025

    Choose a reason for hiding this comment

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

    Fix after an unintentional rollback.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    Review effort 1/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @andsimakov