fix: increase timeout for test launch function test to 30 seconds#186
fix: increase timeout for test launch function test to 30 seconds#186jcharkow merged 2 commits intoOpenMS:mainfrom
Conversation
WalkthroughThis pull request focuses on improving the formatting and readability in the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Framework
participant TL as test_launch
participant L as launch.run()
T->>TL: Invoke test_launch()
TL->>L: Call launch.run() with timeout=30s
L-->>TL: Return launch status
TL-->>T: Assert outcome and complete test
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test_gui.py (1)
133-134: Consider documenting the timeout selection.The 60-second timeout for workflow execution seems appropriate. Consider adding a brief comment explaining why this specific timeout value was chosen to help future maintainers understand its significance.
- launch.button[1].click().run(timeout=60) + # Workflow execution can take up to a minute to complete + launch.button[1].click().run(timeout=60) assert not launch.exception
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test_gui.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-full-app
- GitHub Check: build-simple-app
- GitHub Check: build-openms
🔇 Additional comments (5)
test_gui.py (5)
16-17: Good code style improvement.Changes from single quotes to double quotes in dictionary keys maintain better consistency throughout the codebase.
22-39: Nicely improved parameterization formatting.The restructured parameterization for the test cases enhances readability and maintainability. This formatting approach with each test path on its own line makes it easier to add, remove, or modify test cases in the future.
47-61: Good improvement to code readability.The restructured parameterization for documentation tests improves clarity and maintainability.
104-114: Good formatting improvement for test parameterization.The restructured parameterization enhances code readability and maintainability.
41-42:Details
❓ Verification inconclusive
Verify if global timeout increase is necessary.
The timeout has been increased from 10 to 30 seconds for all test cases, while the PR objective specifically mentions addressing timeout issues for the
raw_data_viewer.pypage. Consider making this timeout increase conditional for just the problematic test to avoid unnecessarily slowing down the entire test suite.
🏁 Script executed:
#!/bin/bash # Check if there are other tests that complete well under 30 seconds # Look for previous test runs to see typical execution times # Find any test logs or timing information find . -name "*.log" -o -name "*test-report*" | xargs grep -l "time" | head -n 5Length of output: 80
Action Required: Review Timeout Increase Scope in Tests
It appears that the increased timeout of 30 seconds for all test cases may be unnecessarily impacting tests that complete well under 10 seconds. The PR only addresses timeout issues related to the
raw_data_viewer.pypage. However, due to the lack of log output from the previous timing script, we’re unable to confirm if other tests really need an increased timeout.
- Verify that only the tests for
raw_data_viewer.pyactually require a longer timeout.- Consider making the timeout increase conditional rather than applying it globally in
test_gui.py(lines 41-42).Please manually verify the typical execution times for tests outside of the problematic
raw_data_viewer.pypage. If timing data from logs cannot be reliably retrieved, re-run additional checks or examine test reports locally to ensure that adjusting the timeout for all tests isn’t inadvertently slowing down the overall test suite.
jcharkow
left a comment
jcharkow
left a comment
There was a problem hiding this comment.
Good catch! As AI suggested increasing the timeout in other places (see above) would be good to do as well.
|
Hello , @jcharkow Thank you for your feedback. |
jcharkow
left a comment
jcharkow
left a comment
There was a problem hiding this comment.
Looks good ready to merge
2 participants
This PR resolves a timeout issue in the test suite where the
test_launchfunction was failing for theraw_data_viewer.pypage due to insufficient loading time. The default 3-second timeout was not enough for the page to fully load and process the example data files.Changes Made:
test_launchfunction in test_gui.py to conditionally increase the timeout to 30 seconds for theraw_data_viewer.pytest.Summary by CodeRabbit