Fix testing regression #10
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @ipv1337, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses a testing regression by adding a script to identify hanging tests and modifying the test execution scripts to include timeouts. The changes aim to prevent tests from running indefinitely, which can block the CI pipeline. I've added a new script scripts/find_hanging_tests.sh that runs tests individually with a timeout. I've also updated scripts/run_coverage_ci.sh and scripts/test_coverage_local.sh to run tests in smaller batches with timeouts and to clean up __pycache__ and .pyc files. Finally, I've removed an unnecessary comment from pytest.ini.
Highlights
- Hanging Test Detection: Introduces a new script (
scripts/find_hanging_tests.sh) to identify tests that hang during execution. This script runs each test file individually with a timeout, reporting any tests that exceed the limit. - Test Execution Timeouts: Modifies the CI and local test coverage scripts (
scripts/run_coverage_ci.shandscripts/test_coverage_local.sh) to include timeouts for individual test files. This prevents the entire test suite from stalling due to a single hanging test. - Cache Cleanup: The CI and local test coverage scripts now include commands to remove
__pycache__directories and.pycfiles before running tests. This ensures a clean test environment and avoids potential import conflicts. - Test Batching: The CI and local test coverage scripts now run tests in smaller batches. This approach helps isolate issues and provides more granular feedback during test execution.
Changelog
Click here to see the changelog
- pytest.ini
- Removed a redundant comment on line 15.
- scripts/find_hanging_tests.sh
- Added a new script to find hanging tests by running each test file individually with a timeout.
- The script cleans up cache files before running tests.
- The script defines a function
run_test_with_timeoutto execute tests with a specified timeout. - The script iterates through a list of test files and reports whether each test completed successfully, timed out, or failed with an exit code.
- scripts/run_coverage_ci.sh
- Modified to include timeouts for individual test files to prevent hanging.
- Added commands to clean up
__pycache__directories and.pycfiles on lines 13 and 14. - The tests are now run in smaller batches, with separate commands for tools tests, model tests, gemini model tests, ollama model tests, config tests, main tests, and remaining tests.
- A warning message is displayed if a test times out or fails.
- scripts/test_coverage_local.sh
- Modified to include timeouts for individual test files to prevent hanging.
- Added commands to clean up
__pycache__directories and.pycfiles on lines 13 and 14. - The tests are now run in smaller batches, with separate commands for tools tests, model tests, gemini model tests, ollama model tests, config tests, main tests, and remaining tests.
- A warning message is displayed if a test times out or fails.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A test that hangs,
A CI pipeline's bane,
Timeout's swift hand.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces a script to identify hanging tests and adds timeouts to the coverage scripts to prevent them from hanging. This is a good approach to improve the reliability of the test suite. However, there are a few areas that could be improved, such as making the timeouts configurable and improving error handling.
Summary of Findings
- Timeout values in scripts: The scripts
run_coverage_ci.shandtest_coverage_local.shhave timeouts set for individual tests. It's important to ensure these timeouts are appropriate for the expected runtime of the tests to prevent premature termination. Consider making the timeout configurable via an environment variable. - Error handling in coverage scripts: The coverage scripts use
|| echo "Warning: ... timed out or failed"to handle test failures. While this prevents the script from exiting, it might mask important error information. Consider logging the specific error message and exit code for better debugging. - Test file list in
find_hanging_tests.sh: Thefind_hanging_tests.shscript contains a hardcoded list of test files. This list should be automatically generated to ensure all tests are checked and to avoid manual updates.
Merge Readiness
The pull request is not ready to be merged. There are medium and high severity issues that need to be addressed before merging. Specifically, the timeout values in the scripts should be made configurable, the error handling in the coverage scripts should be improved, and the test file list in find_hanging_tests.sh should be automatically generated. I am unable to approve this pull request, and users should have others review and approve this code before merging.
| python_classes = Test* | ||
| python_functions = test_* | ||
| timeout = 30 # Set default timeout for all tests to 30 seconds | ||
| timeout = 30 |
There was a problem hiding this comment.
Removing the comment here makes the purpose of the timeout less clear. Consider adding a brief comment explaining why the timeout is set, or referring to documentation where the timeout strategy is described.
timeout = 30 # Set default timeout for all tests to 30 seconds
scripts/find_hanging_tests.sh
Outdated
| find . -name "*.pyc" -exec rm -f {} + 2>/dev/null || true | ||
|
|
||
| # Set timeout (in seconds) | ||
| TIMEOUT=15 |
There was a problem hiding this comment.
Consider making the timeout configurable via an environment variable. This would allow users to adjust the timeout based on their specific needs and hardware. Also, consider adding a comment about the units of the timeout value (seconds).
| TIMEOUT=15 | |
| TIMEOUT=${TIMEOUT:-15} # Timeout in seconds, default to 15 if not set |
scripts/run_coverage_ci.sh
Outdated
| --cov=src.cli_code \ | ||
| --cov-append \ | ||
| --timeout=60 \ | ||
| "$test_file" || echo "Warning: $test_file timed out or failed" |
There was a problem hiding this comment.
Using || echo "Warning: $test_file timed out or failed" will prevent the script from exiting if a test fails, but it might mask important error information. Consider logging the specific error message and exit code for better debugging. You can capture the exit code using $? immediately after the pytest command and include it in the warning message.
| "$test_file" || echo "Warning: $test_file timed out or failed" | |
| "$test_file" || { echo "Warning: $test_file timed out or failed with exit code $?"; } |
scripts/test_coverage_local.sh
Outdated
| --cov=src.cli_code \ | ||
| --cov-append \ | ||
| --timeout=30 \ | ||
| "$test_file" || echo "Warning: $test_file timed out or failed" |
There was a problem hiding this comment.
Using || echo "Warning: $test_file timed out or failed" will prevent the script from exiting if a test fails, but it might mask important error information. Consider logging the specific error message and exit code for better debugging. You can capture the exit code using $? immediately after the pytest command and include it in the warning message.
| "$test_file" || echo "Warning: $test_file timed out or failed" | |
| "$test_file" || { echo "Warning: $test_file timed out or failed with exit code $?"; } |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…d auto-generate test file list
…ling, and add detailed logs
Code Coverage Report📊 Current Coverage: 85.00% Detailed coverage analysis is available in SonarCloud Coverage Change DetailsThis shows code coverage for changes in this PR. To improve coverage, consider adding tests for new or modified code. |
|
Add script to identify hanging tests