Tr/run a single test - Githubissues

mrT23 commented 2 weeks ago

PR Type

enhancement, configuration changes

Description

Implemented support for running a single test by modifying test commands in CoverAgent.
Added timeout handling for command execution in Runner using subprocess.run with a configurable timeout.
Introduced a new CLI argument --run-each-test-separately to control test execution mode.
Updated configuration to include max_allowed_runtime_seconds for test command timeouts.

Changes walkthrough 📝

Relevant files

Enhancement

CoverAgent.py `Add single test execution support in CoverAgent` cover_agent/CoverAgent.py Added method `parse_command_to_run_only_a_single_test` to modify test commands for single test execution. Integrated single test execution logic into the constructor.	+16/-0
Runner.py `Implement command timeout handling in Runner` cover_agent/Runner.py Added timeout handling for command execution using `subprocess.run`. Integrated `max_allowed_runtime_seconds` from settings for timeout.	+13/-5

Configuration changes

utils.py `Add CLI argument for separate test execution` cover_agent/utils.py Added `--run-each-test-separately` argument for test execution mode. Updated default value for `--max-iterations`.	+9/-1
configuration.toml `Update configuration with test timeout setting` cover_agent/settings/configuration.toml - Added `max_allowed_runtime_seconds` setting under `[tests]`.	+4/-1

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

codiumai-pr-agent-pro[bot] commented 2 weeks ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 Security concerns

Command Injection:
The test_command is being modified and executed using subprocess.run with shell=True. While the command comes from args, any malicious input in test_file_relative_path could potentially lead to command injection. The code should sanitize or validate the file path before using it in command construction.

⚡ Recommended focus areas for review

Code Duplication
The test_file_path parameter is duplicated in the UnitTestGenerator constructor call Incomplete Implementation
The unittest case handling is left as a pass statement without implementation, which could cause issues if unittest commands are used Default Value Concern
Setting run-each-test-separately default to True may impact performance as it will run tests individually by default

hussam789 commented 2 weeks ago

PR Code Suggestions ✨

Latest suggestions up to 8b263b2

Previous suggestions

Suggestions up to commit 8b263b2

Category	Suggestion	Score
Possible issue	Add error handling for string operations that may fail if expected substrings are not found ___ The string slicing operation to find the '--' index is unsafe as it assumes '--' exists in the command. Add error handling to prevent potential IndexError. [cover_agent/CoverAgent.py [53-55]](https://github.com/Codium-ai/cover-agent/pull/208/files#diff-dac868643df79ec6dfcf446359468a88ba6a6617bb8ffa0139757f0fbf5195b1R53-R55) ```diff -ind1 = test_command.index('pytest') -ind2 = test_command[ind1:].index('--') -args.test_command = f"{test_command[:ind1]}pytest {test_file_relative_path} {test_command[ind1 + ind2:]}" +try: + ind1 = test_command.index('pytest') + ind2 = test_command[ind1:].index('--') + args.test_command = f"{test_command[:ind1]}pytest {test_file_relative_path} {test_command[ind1 + ind2:]}" +except ValueError: + args.test_command = f"pytest {test_file_relative_path}" ``` Suggestion importance[1-10]: 8 Why: The suggestion addresses a critical issue where the code could crash if '--' is not found in the test command. The error handling provides a sensible fallback behavior.	8
Possible issue	Use appropriate argument parser action for boolean flags to properly handle command-line inputs ___ The `--run-each-test-separately` argument uses bool type which doesn't properly handle command line inputs. Use `store_true` action instead. [cover_agent/utils.py [173-178]](https://github.com/Codium-ai/cover-agent/pull/208/files#diff-4b68afa4ecc709b261a42ca40bfe986f4d150bf47184bdecdadc4954d434dcb0R173-R178) ```diff parser.add_argument( "--run-each-test-separately", - type=bool, + action="store_true", default=True, help="Run each test separately. Default: True" ) ``` Suggestion importance[1-10]: 7 Why: Using type=bool for command line arguments is problematic as it doesn't properly parse command-line inputs. The store_true action is the correct approach for boolean flags.	7
General	Provide meaningful output messages for timeout errors instead of empty strings ___ The timeout error handler returns an empty string for stdout which may cause issues in code expecting output. Consider returning a meaningful error message in stdout. [cover_agent/Runner.py [33-34]](https://github.com/Codium-ai/cover-agent/pull/208/files#diff-7c6d19556f57d99df538c28e384380f1a12da8b57a1e9e382175a8dbdae6c947R33-R34) ```diff except subprocess.TimeoutExpired: - # Handle the timeout case - return "", "Command timed out", -1, command_start_time + error_msg = f"Command execution timed out after {max_allowed_runtime_seconds} seconds" + return error_msg, error_msg, -1, command_start_time ``` Suggestion importance[1-10]: 6 Why: The suggestion improves error handling by providing clear error messages in both stdout and stderr, making it easier to diagnose timeout issues.	6

Suggestions up to commit c6f31b3

Category	Suggestion	Score
General	Remove duplicate parameter that could cause confusion or override behavior ___ Remove the duplicate test_file_path parameter in the UnitTestGenerator constructor call to avoid confusion and potential issues. [cover_agent/CoverAgent.py [32-35]](https://github.com/Codium-ai/cover-agent/pull/208/files#diff-dac868643df79ec6dfcf446359468a88ba6a6617bb8ffa0139757f0fbf5195b1R32-R35) ```diff self.test_gen = UnitTestGenerator( source_file_path=args.source_file_path, test_file_path=args.test_file_output_path, - test_file_path=args.test_file_output_path, ``` Suggestion importance[1-10]: 9 Why: The duplicate parameter is a clear bug that could cause unexpected behavior or confusion. Removing it is essential for code correctness and maintainability.	9
General	Improve error reporting for timeout scenarios with detailed logging ___ Add logging for timeout events to help diagnose performance issues and ensure proper error tracking. [cover_agent/Runner.py [32-34]](https://github.com/Codium-ai/cover-agent/pull/208/files#diff-7c6d19556f57d99df538c28e384380f1a12da8b57a1e9e382175a8dbdae6c947R32-R34) ```diff except subprocess.TimeoutExpired: - # Handle the timeout case - return "", "Command timed out", -1, command_start_time + error_msg = f"Command timed out after {max_allowed_runtime_seconds} seconds" + logging.error(error_msg) + return "", error_msg, -1, command_start_time ``` Suggestion importance[1-10]: 6 Why: Adding detailed logging for timeout events would improve debugging and monitoring capabilities, though the current implementation is functional without it.	6
Possible issue	Add error handling for string parsing operations to prevent crashes ___ Add error handling for the case when the test command string doesn't contain the expected substring 'pytest' or when string indexing fails. [cover_agent/CoverAgent.py [52-55]](https://github.com/Codium-ai/cover-agent/pull/208/files#diff-dac868643df79ec6dfcf446359468a88ba6a6617bb8ffa0139757f0fbf5195b1R52-R55) ```diff if 'pytest' in test_command: - ind1 = test_command.index('pytest') - ind2 = test_command[ind1:].index('--') - args.test_command = f"{test_command[:ind1]}pytest {test_file_relative_path} {test_command[ind1 + ind2:]}" + try: + ind1 = test_command.index('pytest') + ind2 = test_command[ind1:].index('--') + args.test_command = f"{test_command[:ind1]}pytest {test_file_relative_path} {test_command[ind1 + ind2:]}" + except ValueError: + self.logger.error("Failed to parse pytest command string") + return ``` Suggestion importance[1-10]: 8 Why: The string parsing operations could fail if the command format is unexpected, potentially causing crashes. Adding proper error handling is crucial for robustness.	8

Codium-ai / cover-agent