test: integration test robustness — stubs, retries, async, diagnostics - #597
Conversation
DummyTreeTUI was missing advance_simple_progress() and stop_live() methods that _process_tree_events in topic_manager now calls.
Summary of ChangesHello @scp7, 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! This pull request resolves an issue in the integration test suite by updating a mock object to include methods that were recently added to the production code. This ensures that the tests accurately reflect the current interface and functionality, preventing potential test failures due to incomplete mock implementations and maintaining the reliability of the test suite. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly adds missing stub methods, advance_simple_progress() and stop_live(), to the DummyTreeTUI mock class in an integration test, resolving a crash. The change is straightforward and effective. I've included one suggestion to further improve the test's robustness by not only stubbing one of the new methods but also verifying that it's called, which would provide stronger guarantees about the component's behavior.
- Add pytest-rerunfailures and pytest-asyncio dependencies - Configure asyncio_mode=auto for native async test support - Replace asyncio.run() wrappers with native async test methods - Move @requires_openai/@requires_gemini to class level - Use shared openai_config fixture in tree tests (remove hardcoded constants) - Add assert_generation_result/assert_topic_build_result diagnostic helpers - Add @pytest.mark.flaky retries to all LLM-dependent tests - Suppress httpx "Event loop is closed" cleanup noise in conftest - Fix false-alarm "Topic paths (0) < expected samples" warning in generator - Improve make test-integration-verbose with -s, --tb=long, --durations
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the robustness and maintainability of the integration test suite. Key changes include adding pytest-rerunfailures for flaky tests, converting synchronous asyncio.run calls to native async def tests, and introducing detailed diagnostic assertion helpers. The refactoring to use shared fixtures and class-level markers also enhances code clarity. Overall, these are excellent improvements. I've added a couple of minor suggestions to further refine the new helper functions.
|
|
||
| @pytest.fixture | ||
| def tree_builder(self): | ||
| def tree_builder(self, openai_config): |
There was a problem hiding this comment.
Summary
DummyTreeTUIstub methods (advance_simple_progress,stop_live) that broke tree integration testspytest-rerunfailuresandpytest-asynciodependencies for flaky test retries and native async supportasyncio.run()wrappers with nativeasync deftest methods (asyncio_mode = "auto")@requires_openai/@requires_geminiskip markers to class levelopenai_configfixture in tree tests (remove hardcoded provider constants)assert_generation_result/assert_topic_build_resultdiagnostic helpers that surface failure details@pytest.mark.flakyretries to all LLM-dependent integration testsmake test-integration-verbosewith-s,--tb=long,--durationsTest plan
make lintpassesmake test(unit tests) passes — 528 testsmake test-integrationwithOPENAI_API_KEYset — all OpenAI tests pass