Feat/pipeline refinements#28
Conversation
There was a problem hiding this comment.
Code Review
This pull request optimizes the test suite and refactors imports to improve execution speed and organization. Key changes include adding parallel execution and timeout support via pytest-xdist and pytest-timeout, marking slow and sandbox tests, deferring heavy imports (like matplotlib and YFinanceProvider), and introducing a session-scoped synthetic_prices fixture. Feedback focuses on improving test implementation details, such as using @pytest.mark.usefixtures to avoid unused arguments, increasing the global test timeout to prevent flaky CI runs, and utilizing the new session-scoped fixture in test_holdout_cap.py to avoid redundant data generation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @pytest.mark.slow | ||
| def test_characterization_orchestrator_run(tmp_path: Path, mock_validate_candidate_pass: None) -> None: # noqa: ARG001 |
There was a problem hiding this comment.
Instead of passing mock_validate_candidate_pass as an unused argument and silencing the linter with # noqa: ARG001, you can use the @pytest.mark.usefixtures decorator. This is the standard and cleaner way in pytest to apply fixtures that are only needed for their side effects.
| @pytest.mark.slow | |
| def test_characterization_orchestrator_run(tmp_path: Path, mock_validate_candidate_pass: None) -> None: # noqa: ARG001 | |
| @pytest.mark.slow | |
| @pytest.mark.usefixtures("mock_validate_candidate_pass") | |
| def test_characterization_orchestrator_run(tmp_path: Path) -> None: |
| ] | ||
|
|
||
| [tool.pytest.ini_options] | ||
| timeout = 10 |
There was a problem hiding this comment.
Setting a global test timeout of 10 seconds (timeout = 10) might be too aggressive for slow E2E tests, especially when running in resource-constrained CI/CD environments where execution can be significantly slower than on local machines. Consider increasing this limit (e.g., to 30 or 60 seconds) or removing the global timeout and applying it selectively to avoid flaky test failures.
| timeout = 10 | |
| timeout = 30 |
| @pytest.mark.slow | ||
| def test_holdout_peek_limit_early_termination( | ||
| project_root_with_lessons: Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| mock_validate_candidate_pass: None, # noqa: ARG001 | ||
| ) -> None: |
There was a problem hiding this comment.
You've defined a session-scoped synthetic_prices fixture in tests/conftest.py to optimize performance, but this test is still calling the local helper _make_synthetic_prices() on line 55. You should inject the synthetic_prices fixture into the test function instead to avoid regenerating the dataset and speed up the test suite. Then, you can remove the local synthetic_prices = _make_synthetic_prices() call on line 55.
| @pytest.mark.slow | |
| def test_holdout_peek_limit_early_termination( | |
| project_root_with_lessons: Path, | |
| monkeypatch: pytest.MonkeyPatch, | |
| mock_validate_candidate_pass: None, # noqa: ARG001 | |
| ) -> None: | |
| @pytest.mark.slow | |
| @pytest.mark.usefixtures("mock_validate_candidate_pass") | |
| def test_holdout_peek_limit_early_termination( | |
| project_root_with_lessons: Path, | |
| monkeypatch: pytest.MonkeyPatch, | |
| synthetic_prices: pd.DataFrame, | |
| ) -> None: |
No description provided.