Revert "[WIP] ci: add more testcase"#27
Conversation
Summary of ChangesHello @zhaochenyang20, 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 primarily reverts a previous commit that introduced additional test cases. It also refactors the router initialization process to handle worker video support asynchronously. The changes streamline the testing process and adjust the router's worker management. Highlights
Changelog
Ignored Files
Activity
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 primarily reverts previous changes related to adding more test cases, specifically removing end-to-end tests and their associated infrastructure. It also includes some refactoring in the CLI and router logic, such as moving the worker video support probing to the CLI startup and improving test organization. While the revert itself is straightforward, some of the refactoring has led to a reduction in test coverage for core router functionalities and CLI argument parsing. It's important to restore this coverage to ensure the stability and correctness of the application.
I am having trouble creating individual review comments. Click here to see my feedback.
tests/unit/test_diffusion_router.py (156-296)
The removal of several test classes (TestRegisterWorker, TestBuildProxyResponse, TestSanitizeResponseHeaders, TestTryDecodeJson, TestConstructor) from this file results in a critical loss of unit test coverage for core DiffusionRouter functionalities. While some worker registration tests were moved to test_router_endpoints.py, crucial aspects like _build_proxy_response (JSON handling, status code preservation), _sanitize_response_headers, _try_decode_json, and various constructor behaviors (e.g., timeout defaults, route registration) are no longer explicitly tested. This significantly increases the risk of regressions. Please ensure that all previously covered functionalities are adequately tested, either by restoring these tests or by adding equivalent coverage in other test files.
tests/unit/test_cli.py (40-47)
The removal of these arguments from the test_parses_worker_urls (formerly test_full_args) method reduces test coverage for the CLI argument parsing. While test_defaults covers default values, this test was responsible for verifying that non-default values for --timeout, --max-connections, --health-check-interval, and --health-check-failure-threshold are correctly parsed. This is a regression in test coverage that should be addressed.
tests/unit/test_cli.py (57-66)
The removal of assertions for args.timeout and the tests for test_rejects_invalid_routing_algorithm and test_accepts_all_valid_algorithms represents a significant loss of test coverage for CLI argument parsing and validation. It's crucial to ensure that the CLI correctly handles these parameters and validates routing algorithms. Please restore these tests to prevent potential regressions.
Reverts #25