✅ Refactor tests, consolidate into a single test file for multiple variants #1409
+4,119
−15,550
Conversation
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
This commit consolidates multiple version-specific test files (for Python 3.8, 3.9, 3.10) into single test files for a significant portion of the documentation examples.
**Summary of Changes:**
The primary goal was to have one test file per example, using pytest parametrization to handle different Python versions of the source documentation files. I achieved this by:
1. **Identifying Groups:** Scanned `tests/test_advanced` and `tests/test_tutorial` to find sets of test files like `test_example.py`, `test_example_py39.py`, `test_example_py310.py`.
2. **Consolidation Strategy:**
* Chose the base file (e.g., `test_example.py`) as the consolidated file.
* Introduced a `pytest` fixture (usually named `module` or `modules`) within the consolidated file.
* This fixture is parametrized with the base name of the example and its versioned counterparts (e.g., "tutorial001", "tutorial001_py39", "tutorial001_py310").
* Used `importlib.import_module()` within the fixture to load the correct example code from `docs_src/` based on the pytest parameter.
* Applied `needs_py39` and `needs_py310` marks (from `tests.conftest`) to the relevant parameters to ensure tests are skipped on incompatible Python versions.
* Modified test functions to accept this new fixture.
* For tests involving FastAPI, adapted existing `session` and `client` fixtures (or created new ones) to correctly use the parametrized `module` for setting up the test environment (in-memory SQLite engine, creating tables, and configuring the `TestClient` with the correct app instance). This often involved reloading the module and ensuring `SQLModel.metadata` was cleared between parametrized runs using the `clear_sqlmodel` fixture.
* For tests that check printed output, the `print_mock` fixture was used. For others, assertions were based on database state (via `sqlalchemy.inspect`) or API responses.
* Deleted the now-redundant version-specific test files.
**Examples Consolidated So Far:**
* **Advanced:**
* `decimal/tutorial001`
* `uuid/tutorial001`
* `uuid/tutorial002`
* **Tutorial - Code Structure:**
* `code_structure/tutorial002`
* **Tutorial - Connect:**
* `connect/create_connected_tables/tutorial001`
* `connect/delete/tutorial001`
* `connect/insert/tutorial001`
* `connect/select/tutorial003`, `tutorial004`, `tutorial005`
* `connect/update/tutorial001`
* **Tutorial - Create DB and Table:**
* `create_db_and_table/tutorial001`, `tutorial002`, `tutorial003`
* **Tutorial - FastAPI:**
* `fastapi/app_testing/tutorial001_tests_main` (refactored from subprocess)
* `fastapi/delete/tutorial001`
* `fastapi/limit_and_offset/tutorial001`
* `fastapi/multiple_models/tutorial001`, `tutorial002`
* `fastapi/read_one/tutorial001`
* `fastapi/relationships/tutorial001`
* `fastapi/response_model/tutorial001`
* `fastapi/session_with_dependency/tutorial001`
* `fastapi/simple_hero_api/tutorial001`
* `fastapi/teams/tutorial001`
This work is part of an effort to simplify the test suite structure.
The next steps would involve continuing this consolidation for the remaining examples. I also received feedback to remove extra comments and consistently use `from types import ModuleType` for type hinting, which I will apply in future work.
This commit represents the completed code modifications for consolidating multiple version-specific test files (for Python 3.8, 3.9, 3.10) into single test files for nearly all documentation examples.
**Summary of Actions:**
1. **Comprehensive File Identification:** I identified all test files in `tests/test_advanced` and `tests/test_tutorial` that followed the pattern of version-specific suffixes (e.g., `_py39.py`, `_py310.py`) for consolidation.
2. **Consolidation Implementation:**
* My primary strategy involved modifying the base test file (e.g., `test_example.py`).
* I introduced a `pytest` fixture, typically named `module`. This fixture is parametrized to load the base version of the example code and its Python version-specific variants from the `docs_src` directory using `importlib.import_module`.
* I applied `needs_py39` and `needs_py310` marks from `tests.conftest` to the relevant parameters to control test execution based on the Python version.
* I updated test functions to use this `module` fixture. For FastAPI examples, this included careful adaptation of `session` and `client` fixtures to use the parametrized module's `app` and `engine`, ensuring proper database setup (in-memory SQLite, table creation) and module reloading with `clear_sqlmodel` for isolation.
* I used the `print_mock` fixture for tests verifying console output. Other tests used `sqlalchemy.inspect` or API response assertions.
* I incorporated your feedback regarding the use of `from types import ModuleType` for type hints and removal of unnecessary comments into later consolidations.
* I deleted redundant version-specific test files after their logic was merged.
3. **Skipped File:** I did not consolidate `tests/test_tutorial/test_insert/test_tutorial002.py` due to persistent `ImportError`/`AttributeError` issues when trying to access a dependent `Team` model from another tutorial's source file within the pytest fixture. Multiple approaches to resolve this failed, suggesting a complex interaction with module loading or metadata in the test environment for this specific case.
4. **Testing Limitations (CRITICAL):**
* While I often ran tests for individual files or smaller directories successfully after consolidation, a persistent "The command affected too many files in the repo" error plagued testing of larger directories and the entire project.
* This environment constraint ultimately **prevented me from executing the full test suite** after all code modifications were complete. Dependency installation (`pip install -r requirements.txt`) also failed due to this limit in the final stages.
* **Therefore, the submitted code, while structurally complete according to my plan, is NOT FULLY TESTED.** There is a risk that consolidations in the later-processed, larger directories might contain unfound issues.
**Conclusion:**
The code refactoring to consolidate tests is (almost entirely) complete. However, due to critical environment limitations preventing full test suite verification, this submission should be reviewed with caution. Further testing in an unrestricted environment is highly recommended.
… done so far and provide feedback for Jules to continue.
This commit applies minor cleanups to the previously consolidated test files. The primary goal of this session was to address your feedback regarding the removal of unnecessary comments and ensuring the correct use of `from types import ModuleType`.
**Summary of Actions during this cleanup session:**
1. **Plan Re-evaluation:** After the initial consolidation work, I created a new plan to systematically review all changed files in batches.
2. **Batch Processing:** I went through the following batches of consolidated test files:
* Batch 1: `tests/test_advanced` and `tests/test_tutorial/test_code_structure`
* Batch 2: `tests/test_tutorial/test_connect`
* Batch 3: `tests/test_tutorial/test_create_db_and_table`
* Batch 4: `tests/test_tutorial/test_fastapi/test_app_testing` and `.../test_delete`
* Batch 5: `.../test_limit_and_offset` and `.../test_multiple_models`
* Batch 6: `.../test_read_one` and `.../test_relationships`
* Batch 7: `.../test_response_model` and `.../test_session_with_dependency`
* Batch 8 (partially): `.../test_teams/test_tutorial001.py` was processed. `.../test_simple_hero_api/test_tutorial001.py` was identified as missed before this submission.
3. **Cleanup Operations:** For each file in the processed batches:
* I checked for and removed superfluous comments (e.g., commented-out code that was no longer relevant, self-explanatory comments). Many files were already quite clean.
* I ensured `from types import ModuleType` was added if `ModuleType` was used as a type hint for a function parameter (typically the `module` fixture).
* I corrected type hints from `type` to `types.ModuleType` where applicable.
4. **Testing Limitations:** Throughout this cleanup session, I encountered an error indicating "The command affected too many files in the repo". This prevented me from verifying that the cleanups did not introduce regressions. The changes are based on visual inspection and targeted modifications.
**Unfinished Work:**
* The cleanup for `tests/test_tutorial/test_fastapi/test_simple_hero_api/test_tutorial001.py` was missed in Batch 8.
* Batches 9 through 19 of the cleanup plan, covering the remaining FastAPI subdirectories, and the general `test_insert`, `test_limit_and_offset`, `test_many_to_many`, `test_one`, `test_relationship_attributes`, and `test_where` directories, were not started.
This submission includes the cleanups made up to the partial completion of Batch 8. Further cleanup and full verification are still pending.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
WIP with Jules
Summary of Changes:
The primary goal was to have one test file per example, using pytest parametrization to handle different Python versions of the source documentation files. I achieved this by:
Identifying Groups: Scanned
tests/test_advancedandtests/test_tutorialto find sets of test files liketest_example.py,test_example_py39.py,test_example_py310.py.Consolidation Strategy:
test_example.py) as the consolidated file.pytestfixture (usually namedmoduleormodules) within the consolidated file.importlib.import_module()within the fixture to load the correct example code fromdocs_src/based on the pytest parameter.needs_py39andneeds_py310marks (fromtests.conftest) to the relevant parameters to ensure tests are skipped on incompatible Python versions.sessionandclientfixtures (or created new ones) to correctly use the parametrizedmodulefor setting up the test environment (in-memory SQLite engine, creating tables, and configuring theTestClientwith the correct app instance). This often involved reloading the module and ensuringSQLModel.metadatawas cleared between parametrized runs using theclear_sqlmodelfixture.print_mockfixture was used. For others, assertions were based on database state (viasqlalchemy.inspect) or API responses.Examples Consolidated So Far:
decimal/tutorial001uuid/tutorial001uuid/tutorial002code_structure/tutorial002connect/create_connected_tables/tutorial001connect/delete/tutorial001connect/insert/tutorial001connect/select/tutorial003,tutorial004,tutorial005connect/update/tutorial001create_db_and_table/tutorial001,tutorial002,tutorial003fastapi/app_testing/tutorial001_tests_main(refactored from subprocess)fastapi/delete/tutorial001fastapi/limit_and_offset/tutorial001fastapi/multiple_models/tutorial001,tutorial002fastapi/read_one/tutorial001fastapi/relationships/tutorial001fastapi/response_model/tutorial001fastapi/session_with_dependency/tutorial001fastapi/simple_hero_api/tutorial001fastapi/teams/tutorial001This work is part of an effort to simplify the test suite structure. The next steps would involve continuing this consolidation for the remaining examples. I also received feedback to remove extra comments and consistently use
from types import ModuleTypefor type hinting, which I will apply in future work.