fix: eliminate backup service I/O from API test lifecycle#1015
fix: eliminate backup service I/O from API test lifecycle#1015
Conversation
create_app auto-builds a real BackupService from config, which triggers filesystem I/O (backup creation + retention pruning of tar.gz archives) on every TestClient startup and shutdown. On Windows each tarfile.open() call gets scanned by Defender (~2 s per archive), adding 15-25 s per test that uses TestClient. With 8 xdist workers this caused: - 10+ minute unit test runs (should be ~90 s) - Worker crashes (gw nodes going down mid-run) - Stalls at ~93% progress (remaining tests stuck on backup I/O) Changes: - Patch build_backup_service to return None in an autouse fixture in the API test conftest, eliminating all backup filesystem I/O from test setup/teardown - Provide a lightweight mock backup service for TestBackupGuards tests that need the service instance in app_state - Add a wall-clock guardrail (8 s) for unit tests in the root conftest that fails any unit test exceeding the limit, preventing future regressions from heavy fixture I/O
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a pytest wall-clock guard in 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches. Re-running this action after a short time may resolve the issue. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
There was a problem hiding this comment.
Code Review
This pull request introduces a wall-clock time limit for unit tests to detect performance regressions caused by heavy I/O and optimizes API unit tests by disabling the backup service by default. It also provides a mock backup service for specific guard tests. A review comment suggests using a more efficient and idiomatic method for checking test markers in the pytest teardown hook.
tests/conftest.py
Outdated
| markers = {m.name for m in item.iter_markers()} | ||
| if "unit" in markers and elapsed > _UNIT_TEST_WALL_CLOCK_LIMIT: |
There was a problem hiding this comment.
Using item.get_closest_marker("unit") is more idiomatic and efficient than constructing a set of all marker names for every test teardown, especially in a large suite with 12,000+ tests. This avoids unnecessary set allocations and iterations over all markers (including parametrization markers) for every test.
| markers = {m.name for m in item.iter_markers()} | |
| if "unit" in markers and elapsed > _UNIT_TEST_WALL_CLOCK_LIMIT: | |
| if item.get_closest_marker("unit") and elapsed > _UNIT_TEST_WALL_CLOCK_LIMIT: |
There was a problem hiding this comment.
Pull request overview
Removes expensive backup-service filesystem I/O from the API unit-test app lifecycle to fix a major test-suite performance regression, and adds a time-based guardrail to prevent similar fixture regressions.
Changes:
- Patch
synthorg.api.app.build_backup_serviceto returnNonefor API unit tests, avoiding backup creation/pruning work duringTestClientstartup/shutdown. - Add a lightweight mocked backup service for HTTP guard tests that require the backup endpoint to respond without performing real I/O.
- Add a global unit-test wall-clock limit (setup + call + teardown) to fail slow unit tests early.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/api/conftest.py | Autouse fixture disables backup service factory for API unit tests to eliminate filesystem I/O. |
| tests/unit/api/controllers/test_backup.py | Adds an autouse mock backup service for guard tests so backup endpoints don’t crash when the global disable is active. |
| tests/conftest.py | Adds pytest hooks to enforce an 8s wall-clock limit for tests marked unit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| @pytest.hookimpl(trylast=True) | ||
| def pytest_runtest_teardown(item: pytest.Item) -> None: |
There was a problem hiding this comment.
pytest_runtest_teardown hook has the wrong signature. Pytest calls this hook with (item, nextitem), so defining it as def pytest_runtest_teardown(item: pytest.Item) -> None will raise TypeError during the first teardown and break the entire test run. Update the hook signature to accept nextitem: pytest.Item | None (and ignore it if unused).
| def pytest_runtest_teardown(item: pytest.Item) -> None: | |
| def pytest_runtest_teardown( | |
| item: pytest.Item, nextitem: pytest.Item | None | |
| ) -> None: |
Replaces set comprehension over all markers with the more efficient and idiomatic get_closest_marker API, avoiding unnecessary set allocation on every test teardown.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1015 +/- ##
=======================================
Coverage 91.66% 91.67%
=======================================
Files 656 656
Lines 35719 35719
Branches 3512 3512
=======================================
+ Hits 32742 32745 +3
Misses 2357 2357
+ Partials 620 617 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🤖 I have created a release *beep* *boop* --- ## [0.5.8](v0.5.7...v0.5.8) (2026-04-03) ### Features * auto-select embedding model + fine-tuning pipeline wiring ([#999](#999)) ([a4cbc4e](a4cbc4e)), closes [#965](#965) [#966](#966) * ceremony scheduling batch 3 -- milestone strategy, template defaults, department overrides ([#1019](#1019)) ([321d245](321d245)) * five-pillar evaluation framework for HR performance tracking ([#1017](#1017)) ([5e66cbd](5e66cbd)), closes [#699](#699) * populate comparison page with 53 competitor entries ([#1000](#1000)) ([5cb232d](5cb232d)), closes [#993](#993) * throughput-adaptive and external-trigger ceremony scheduling strategies ([#1003](#1003)) ([bb5c9a4](bb5c9a4)), closes [#973](#973) [#974](#974) ### Bug Fixes * eliminate backup service I/O from API test lifecycle ([#1015](#1015)) ([08d9183](08d9183)) * update run_affected_tests.py to use -n 8 ([#1014](#1014)) ([3ee9fa7](3ee9fa7)) ### Performance * reduce pytest parallelism from -n auto to -n 8 ([#1013](#1013)) ([43e0707](43e0707)) ### CI/CD * bump docker/login-action from 4.0.0 to 4.1.0 in the all group ([#1027](#1027)) ([e7e28ec](e7e28ec)) * bump wrangler from 4.79.0 to 4.80.0 in /.github in the all group ([#1023](#1023)) ([1322a0d](1322a0d)) ### Maintenance * bump github.com/mattn/go-runewidth from 0.0.21 to 0.0.22 in /cli in the all group ([#1024](#1024)) ([b311694](b311694)) * bump https://github.com/astral-sh/ruff-pre-commit from v0.15.8 to 0.15.9 in the all group ([#1022](#1022)) ([1650087](1650087)) * bump node from `71be405` to `387eebd` in /docker/sandbox in the all group ([#1021](#1021)) ([40bd2f6](40bd2f6)) * bump node from `cf38e1f` to `ad82eca` in /docker/web in the all group ([#1020](#1020)) ([f05ab9f](f05ab9f)) * bump the all group in /web with 3 updates ([#1025](#1025)) ([21d40d3](21d40d3)) * bump the all group with 2 updates ([#1026](#1026)) ([36778de](36778de)) * enable additional eslint-react rules and fix violations ([#1028](#1028)) ([80423be](80423be)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
Fixes a severe test performance regression where the full unit test suite took 10+ minutes with worker crashes and stalls at ~93% progress. Root cause:
create_appauto-builds a realBackupServicethat triggers filesystem I/O (backup creation + retention pruning of tar.gz archives) on everyTestClientstartup and shutdown. On Windows, eachtarfile.open()call gets scanned by Defender (~2s per archive), adding 15-25s per test lifecycle.Changes
tests/unit/api/conftest.py): patchesbuild_backup_serviceto returnNonefor all API unit tests, eliminating backup filesystem I/O from test setup/teardowntests/unit/api/controllers/test_backup.py):TestBackupGuardsneeds a service instance inapp_stateso the list-backups endpoint responds -- provides a lightweight mock that avoids real I/Otests/conftest.py): fails any unit test exceeding 8s total wall-clock time (setup + call + teardown), preventing future regressions from heavy fixture I/OResults
Test plan