Refactor test_main.py towards pytest#329
Closed
johbo wants to merge 51 commits intocecli-dev:mainfrom
Closed
Conversation
Phase 1 of pytest migration: Replace unittest setUp/tearDown with function-scoped autouse fixture. Changes: - Add pytest import - Create test_env autouse fixture that runs for each test - Fixture provides same environment as setUp/tearDown via request.instance - Remove setUp method from TestMain - Remove tearDown method from TestMain - Keep TestCase inheritance (will remove in Phase 2.2) - Keep all test methods unchanged Verification: - All 83 tests pass (83/83) - Tests still use self.tempdir, self.create_env_file, etc. - Fixture handles setup/teardown automatically Why function-scoped not module-level: - Tests create files, repos, modify environment - Need fresh isolation per test - Module-level would share state and break tests Next: Phase 2.1 - Convert assertions to plain assert (keep TestCase)
Phase 2.1 Batch 1: Convert unittest assertions to plain assert for basic main() functionality tests while keeping TestCase base class. Tests converted: - test_main_with_empty_dir_no_files_on_command (no assertions) - test_main_with_emptqy_dir_new_file - test_main_with_empty_git_dir_new_file - test_main_with_empty_git_dir_new_files - test_main_with_dname_and_fname - test_main_with_subdir_repo_fnames - test_main_with_empty_git_dir_new_subdir_file (no assertions) - test_setup_git - test_check_gitignore - test_return_coder - test_main_with_git_config_yml (already had plain assert) Assertions converted: - self.assertTrue(x) → assert x - self.assertFalse(x) → assert not x - self.assertEqual(a, b) → assert a == b - self.assertNotEqual(a, b) → assert a != b (or assert a is not None) - self.assertIsInstance(x, T) → assert isinstance(x, T) Verification: - All 11 tests pass (11/11) - TestCase base class still in place - pytest assertion rewriting tested and working Next: Batch 2 - Environment & configuration tests (20 tests)
Phase 2.1 Batch 2: Convert unittest assertions to plain assert for environment and configuration tests while keeping TestCase base class. Tests converted: - test_env_file_override - test_env_file_flag_sets_automatic_variable - test_default_env_file_sets_automatic_variable - test_false_vals_in_env_file - test_true_vals_in_env_file - test_verbose_mode_lists_env_vars - test_yaml_config_file_loading - test_pytest_env_vars - test_set_env_single - test_set_env_multiple - test_set_env_with_spaces - test_set_env_invalid_format - test_api_key_single - test_api_key_multiple - test_api_key_invalid_format - test_git_config_include - test_git_config_include_directive - test_load_dotenv_files_override Assertions converted: - self.assertEqual(a, b) → assert a == b - self.assertIn(x, y) → assert x in y - self.assertRegex(s, r) → assert re.search(r, s) - self.assertLess(a, b) → assert a < b Verification: - All 18 tests pass (18/18) - TestCase base class still in place Progress: 29/83 tests converted (35%) Next: Batch 3 - Model configuration tests
- test_resolve_aiderignore_path - test_invalid_edit_format - test_default_model_selection - test_model_precedence - test_model_overrides_suffix_applied - test_model_overrides_no_match_preserves_model_name - test_chat_language_spanish - test_commit_language_japanese - test_main_exit_with_git_command_not_found - test_reasoning_effort_option - test_thinking_tokens_option - test_list_models_includes_metadata_models - test_list_models_includes_all_model_sources - test_list_models_with_direct_resource_patch - test_stream_without_cache_no_warning - test_cache_without_stream_no_warning Converted unittest assertions to plain assert statements while keeping TestCase base class. All 83 tests still passing.
- test_command_line_gitignore_files_flag - test_add_command_gitignore_files_flag - test_encodings_arg - test_yes - test_default_yes - test_dark_mode_sets_code_theme - test_light_mode_sets_code_theme - test_lint_option - test_lint_option_with_explicit_files - test_lint_option_with_glob_pattern - test_map_mul_option - test_suggest_shell_commands_default/disabled/enabled - test_detect_urls_default/disabled/enabled - test_accepts_settings_warnings - test_argv_file_respects_git - test_mcp_servers_parsing Converted all remaining unittest assertions to plain assert statements. All 83 tests still passing. Ready to remove TestCase inheritance.
Removed TestCase base class and unittest import. All tests now use pure pytest patterns with plain assert statements and pytest fixtures. All 83 tests passing. Phase 2 (assertion conversion) complete!
Replace 6 separate boolean flag tests with a single parametrized test: - test_suggest_shell_commands_* (3 tests) - test_detect_urls_* (3 tests) Reduces duplication while maintaining all test cases. All 83 tests pass.
Replace 3 separate API key tests with a single parametrized test: - test_api_key_single - test_api_key_multiple - test_api_key_invalid_format All 83 tests pass.
Replace 4 separate --set-env tests with a single parametrized test: - test_set_env_single - test_set_env_multiple - test_set_env_with_spaces - test_set_env_invalid_format All 83 tests pass.
Replace 2 separate mode tests with a single parametrized test: - test_dark_mode_sets_code_theme - test_light_mode_sets_code_theme All 83 tests pass.
Split large test_default_model_selection into: - Parametrized test for 5 API key scenarios (anthropic, deepseek, openrouter, openai, gemini) - Separate test for OAuth fallback when no API keys present All 88 tests pass (83 → 88 due to test expansion).
Replace single test with 5 internal sub-tests with a parametrized test: - test_main_args now tests 5 scenarios via parametrization - no_auto_commits, auto_commits, defaults, no_dirty_commits, dirty_commits All 92 tests pass.
Create dummy_io fixture to eliminate 75+ duplicate DummyInput/Output calls. All tests now use **dummy_io instead of explicit input/output parameters. Reduces duplication and improves test maintainability. All 92 tests pass.
Create mock_coder fixture to eliminate duplicate Coder.create mock setup. Uses pytest-mock's mocker fixture for cleaner mocking. Updated 4 tests to use the new fixture: - test_main_with_git_config_yml - test_main_args (parametrized) - test_false_vals_in_env_file - test_true_vals_in_env_file All 92 tests pass.
Created git_temp_dir fixture to provide temporary git directories: - Added fixture yielding GitTemporaryDirectory as Path object - Added git_temp_dir parameter to 57 test methods that need it - Removed 36 redundant GitTemporaryDirectory() context managers - Tests using self.tempdir (from test_env) excluded from fixture All 92 tests passing.
Created create_env_file factory fixture: - Converted helper method to pytest factory fixture - Uses Path.cwd() to create env files in current test directory - Updated 5 calls across 5 test methods to use fixture - Removed old TestMain.create_env_file method All 92 tests passing.
Converted all 16 @patch decorators to pytest-mock mocker.patch(): - Removed @patch decorators from test methods - Added mocker parameter to test signatures - Replaced unittest.mock.patch with mocker.patch calls - Tests with multiple decorators now use multiple mocker.patch calls All 92 tests passing.
Converted 5 additional `with patch` context managers to pytest-mock: - test_message_file_flag - test_encodings_arg (nested patches) - test_mode_sets_code_theme - test_env_file_flag_sets_automatic_variable - test_default_env_file_sets_automatic_variable Progress: 21/40 total patch usages converted to mocker. All 92 tests passing.
Converted 5 additional `with patch` context managers to pytest-mock: - test_lint_option - test_lint_option_with_explicit_files - test_lint_option_with_glob_pattern - test_map_tokens_option - test_map_tokens_option_with_non_zero_value Progress: 27/40 total patch usages converted to mocker (67.5%). All 92 tests passing.
Converted 2 more `with patch` context managers to pytest-mock: - test_sonnet_and_cache_options (RepoMap) - test_verbose_mode_lists_env_vars (sys.stdout with StringIO) Progress: 29/40 total patch usages converted to mocker (72.5%). All 92 tests passing.
Converted 2 more `with patch` context managers to pytest-mock: - test_invalid_edit_format (sys.stderr with StringIO) - test_list_models_includes_metadata_models (sys.stdout) Progress: 31/40 total patch usages converted (77.5%). All 92 tests passing.
Converted 4 more `with patch` context managers to pytest-mock: - test_list_models_includes_all_model_sources (sys.stdout) - test_check_model_accepts_settings_flag (Model.set_thinking_tokens) - test_list_models_with_direct_resource_patch (3 patches: importlib_resources.files, sys.stdout, Model.set_reasoning_effort) Progress: 36/40 total patch usages converted (90%). All 92 tests passing.
Replace all unittest.mock.patch usage with pytest-mock's mocker: - Converted test_env autouse fixture to use mocker instead of patch - Converted 5 remaining tests using with patch() context managers: - test_main_exit_calls_version_check - test_yaml_config_file_loading - test_accepts_settings_warnings - test_model_overrides_suffix_applied - test_model_overrides_no_match_preserves_model_name - Removed patch import from unittest.mock All 92 tests passing. Code now fully uses pytest-mock for all mocking.
Remove unittest legacy patterns from test_env fixture: - Removed request.instance pattern and all instance variable assignments - Removed request parameter (no longer needed) - Replaced self.tempdir with os.getcwd() in 3 locations: - test_setup_git (2 uses) - test_list_models_with_direct_resource_patch (1 use) - Simplified fixture to only use mocker parameter All 92 tests passing. Fixture is now more idiomatic pytest.
Replace manual environment variable manipulation with pytest's monkeypatch: - test_check_gitignore: Use monkeypatch.setenv for GIT_CONFIG_GLOBAL - test_env_file_override: Use monkeypatch.setenv for HOME and E - test_yaml_config_file_loading: Use monkeypatch.setenv for HOME - test_model_precedence: Use monkeypatch.setenv for API keys Benefits: - Automatic cleanup (no more del os.environ) - More explicit and idiomatic pytest - Cleaner code with fewer lines All 92 tests passing. Phase 3C complete.
Complete transformation to idiomatic pytest: - Removed TestMain class wrapper - Converted all 74 test methods to standalone functions - Removed 'self' parameter from all test signatures - Added comprehensive module docstring - Enhanced test_env fixture documentation Code is now fully idiomatic pytest with: - Function-based tests (no class wrapper) - Pytest fixtures for dependency injection - Parametrized tests for reducing duplication - pytest-mock for all mocking - monkeypatch for environment variables All 92 tests passing.
Merge test_main_smoke.py into test_main.py to eliminate fixture duplication: - Added 2 smoke tests (test_main_executes, test_main_async_executes) - Updated smoke tests to use dummy_io fixture for consistency - Deleted tests/basic/test_main_smoke.py - Updated module docstring to reflect consolidated suite Benefits: - Single source of truth for main() tests - No fixture duplication between files - Simpler test organization Total: 94 tests (92 comprehensive + 2 smoke tests) All tests passing.
Remove test_main_executes as it duplicates existing coverage: - test_main_with_empty_dir_no_files_on_command already tests main() execution - 90+ other tests call main() with various arguments - No unique value added by the redundant smoke test Keep test_main_async_executes as it's the ONLY test for main_async(). Applying clean code principles: avoid test duplication. Total: 93 tests (92 comprehensive + 1 async smoke test) All tests passing.
Remove test_main_async_executes as it provides zero additional coverage: - main() is just a thin wrapper: asyncio.run(main_async(...)) - All 92 tests calling main() already test main_async() indirectly - All business logic lives in main_async(), tested via main() Updated module docstring to clarify that tests cover both entry points. Clean code principle: eliminated all test duplication. Total: 92 comprehensive tests All tests passing.
Major improvements for readability, maintainability, and test clarity: **Priority 1 - Critical Fixes:** - Split test_list_models_with_direct_resource_patch into two separate tests (was testing two completely unrelated behaviors) - Remove debug print statement from test_yaml_config_file_loading **Priority 2 - Complexity Reduction:** - Parametrize test_accepts_settings_warnings (81 lines → 65 lines) 4 scenarios now explicit and independently testable - Split test_yaml_config_file_loading into 4 independent tests (62 lines → 4 focused tests, eliminates sequential dependencies) - Add assert_warning_contains() helper for consistent warning verification **Priority 3 - Eliminate Logical Duplication:** - Consolidate gitignore flag tests (107 lines → 53 lines) 6 parametrized test cases covering both command-line and /add command - Consolidate env file variable tests (40 lines → 43 lines) 4 parametrized test cases for dark mode and boolean parsing - Consolidate cache/streaming warning tests (33 lines → 26 lines) 3 parametrized test cases for flag combinations **Test Count Changes:** - Before: 92 tests (some monolithic) - After: 103 tests (better granularity) - Net: +11 tests from parametrization (better debugging) **LOC Changes:** - Before: 1,715 lines - After: 1,694 lines (-21 lines) - Reduction: ~1.2% **Clean Code Principles Applied:** - Single Responsibility: Each test now tests one clear behavior - DRY: Eliminated duplicate test setup code - Readability: Clear parametrize IDs make test intent obvious - Maintainability: Helper function for common assertions All 103 tests pass ✓
Remove unnecessary `with GitTemporaryDirectory()` context managers from tests that already use the `git_temp_dir` fixture. The fixture already creates a temp directory and changes into it (via ChdirTemporaryDirectory), so using the context manager again creates a redundant nested temp directory. Tests refactored: - test_yaml_config_loads_from_named_file - test_yaml_config_loads_from_cwd - test_yaml_config_loads_from_git_root - test_yaml_config_loads_from_home - test_git_config_include - test_git_config_include_directive - test_model_overrides_suffix_applied - test_model_overrides_no_match_preserves_model_name - test_env_file_override - test_lint_option - test_load_dotenv_files_override - test_mcp_servers_parsing - test_gitignore_files_flag All 103 tests pass.
Replace manual os.environ manipulation in test_env fixture with mocker.patch.dict(os.environ, clean_env, clear=True) for complete environment isolation. Improvements: - Completely replaces os.environ instead of modifying it - Automatic cleanup via mocker (no manual restore needed) - Add Windows compatibility (USERPROFILE vs HOME) - More idiomatic pytest-mock usage - Cleaner, more maintainable code Pattern adopted from the old test_main_smoke.py isolated_env fixture. All 103 tests pass.
Use 'with' statements for both temporary directories to leverage automatic cleanup via __exit__. More idiomatic and eliminates manual cleanup() calls. All 103 tests pass.
Replace IgnorantTemporaryDirectory + manual os.chdir() calls with ChdirTemporaryDirectory, which automatically: - Changes to temp directory in __enter__ - Changes back to original directory in __exit__ Eliminates both manual os.chdir(tempdir) and os.chdir(original_cwd) calls. Cleaner and leverages the inheritance hierarchy correctly. All 103 tests pass.
Create dedicated temp_home fixture to manage temporary home directory, making it reusable and separating concerns. The test_env fixture now depends on temp_home for cleaner fixture composition. Benefits: - Single responsibility: each fixture manages one resource - Reusable: temp_home can be used by other tests if needed - Cleaner structure and dependency injection All 103 tests pass.
Create dedicated temp_cwd fixture to manage temporary working directory with automatic chdir. Now we have three composable fixtures: - temp_cwd: temporary current working directory (auto chdir) - temp_home: temporary home directory - test_env: composes the above with environment isolation Benefits: - Complete separation of concerns - Each fixture manages exactly one resource - Clean fixture composition through dependency injection - Reusable fixtures for other tests All 103 tests pass.
The yield was serving no purpose since: - No teardown code after yield - All cleanup handled by dependency fixtures (temp_cwd, temp_home) - mocker automatically cleans up all patches This makes it clear that test_env is a setup-only fixture that composes other fixtures for resource management. All 103 tests pass.
Replace create_env_file fixture with direct Path operations since it
was just a trivial wrapper around Path().write_text() with no real
value added:
- No resource management or cleanup
- No fixture dependencies
- No pytest-specific functionality
- Just 2 lines wrapped in complexity
Replaced 2 usages:
1. env_file_path = Path(env_file); env_file_path.write_text(content)
2. Path(".env").write_text("AIDER_DARK_MODE=on")
All 103 tests pass.
Remove dead code that was never called anywhere in the tests. Tests that check warnings do the same logic inline: - test_accepts_settings_warnings (lines 936-940) - test_stream_cache_warning (lines 1543-1544) If a helper is needed in the future, it can be extracted from actual usage patterns rather than maintaining unused code. All 103 tests pass.
Split single test with 'if method' branching into two separate tests: - test_gitignore_files_flag_command_line: Tests CLI argument parsing - test_gitignore_files_flag_add_command: Tests /add command behavior Benefits: - No more if statement for method branching - Clear separation of concerns - Each test has single focus (one way to add files) - Better test names reveal intent - Easier to debug failures - Shared setup via _create_gitignore_test_files() helper Still 6 test cases total (2 methods × 3 flag variations), now with better organization. All 103 tests pass.
Remove boilerplate module docstring that just restates what's obvious from the file name and test names.
- Integrated OpenAI provider registry feature - Refactored new test_list_models_includes_openai_provider to use: * dummy_io and git_temp_dir fixtures * mocker instead of patch context managers * pytest-style assertions (assert instead of self.assertIn) * **dummy_io pattern instead of DummyInput/DummyOutput - Added types import for types.SimpleNamespace - All 104 tests passing
johbo
force-pushed
the
test-main-pytest
branch
from
cc1ddbb to
738302a
Compare
The test_env fixture was using mocker.patch.dict(os.environ, ..., clear=True) which cleared ALL environment variables including PATH. This broke git operations on Windows where git.exe cannot be found without PATH. Since tests are already protected by comprehensive mocking (Coder.create, InputOutput, --exit flags), clearing the environment is unnecessary security theater. The real protection is the mocking layer, not environment isolation. Benefits of this change: - Fixes ~55 test failures on Windows (git command not found) - Allows debugging with environment variables (AIDER_VERBOSE=1, etc.) - Simpler fixture without platform-specific whitelisting - Relies on existing mock boundaries for API call prevention All 104 tests passing on macOS.
- test_main_with_dname_and_fname → test_main_with_subdir_and_fname - test_yes → test_yes_always - test_default_yes → test_default_of_yes_all_is_none Improves test naming consistency and readability.
Replace manual StringIO mocking with pytest's built-in capsys fixture in 6 tests: - test_verbose_mode_lists_env_vars - test_invalid_edit_format (stderr) - test_list_models_includes_metadata_models - test_list_models_includes_all_model_sources - test_list_models_includes_openai_provider - test_list_models_with_direct_resource_patch Removes StringIO import dependency and uses standard pytest pattern for cleaner, more maintainable test code.
Replace tempfile operations with pytest's tmp_path fixture in 2 tests: - test_message_file_flag: Replace tempfile.mktemp() with tmp_path - test_read_option_with_external_file: Replace NamedTemporaryFile with tmp_path Benefits: - Removes tempfile import dependency - Automatic cleanup (no manual os.unlink/os.remove needed) - Safer and more reliable (no deprecated mktemp) - Standard pytest pattern for better maintainability - Eliminates try/finally blocks for cleanup
johbo
changed the title
gopar
reviewed
Dec 31, 2025
|
|
||
|
|
||
| @pytest.fixture | ||
| def git_temp_dir(): |
There was a problem hiding this comment.
We can move these fixtures into their own file since more tests will rely on similar fixtures, instead of copy/pasta all around on tests via conftest.py
Author
There was a problem hiding this comment.
Yes, think this will happen. I've noticed after this PR that quite a few more tests were not properly running and want to first move everything into the pytest style, so that we can be sure tests are executed. And then improve based on that.
It turned out to be a bit more of a challenge than anticipated though :D
Collaborator
|
In v0.90.2! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up work from #327
I've let the AI go a bit crazy on this, apologies upfront for the massive change. Still think it is worth keeping the refactored result.
For other tests which still use unittest I hope that I found a quite simple pattern to convert without a massive refactoring:
assertstatements, example f627b3dunittest.TestCaseinheritance, see 08105e7The result is that the test cases run with plain pytest and also support
async defwithout the need of a special base class.Here is what the AI says about this PR:
Refactor test_main.py fixtures for idiomatic pytest patterns
This PR modernizes
tests/basic/test_main.pyto use more idiomatic pytest patterns, improving maintainability and readability while reducing code duplication.Key Changes
Fixture Improvements:
test_envfixture to usemocker.patch.dict()for complete environment isolation with Windows compatibility (USERPROFILE vs HOME)temp_cwdandtemp_homefixtures for better separation of concernsChdirTemporaryDirectorycontext manager for automatic directory managementCode Cleanup:
with GitTemporaryDirectory()in 13 tests that already used thegit_temp_dirfixturecreate_env_filefixture - replaced with directPath().write_text()callsassert_warning_containshelper (16 lines of dead code)test_gitignore_files_flaginto two focused parametrized tests with shared helpermock_runvariables)Code Quality:
Impact