Skip to content

fix: eliminate backup service I/O from API test lifecycle#1015

Merged
Aureliolo merged 2 commits intomainfrom
fix/test-performance-backup-teardown
Apr 2, 2026
Merged

fix: eliminate backup service I/O from API test lifecycle#1015
Aureliolo merged 2 commits intomainfrom
fix/test-performance-backup-teardown

Conversation

@Aureliolo
Copy link
Copy Markdown
Owner

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_app auto-builds a real BackupService that 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 (~2s per archive), adding 15-25s per test lifecycle.

Changes

  • Autouse fixture to disable backup service (tests/unit/api/conftest.py): patches build_backup_service to return None for all API unit tests, eliminating backup filesystem I/O from test setup/teardown
  • Mock backup service for guard tests (tests/unit/api/controllers/test_backup.py): TestBackupGuards needs a service instance in app_state so the list-backups endpoint responds -- provides a lightweight mock that avoids real I/O
  • Wall-clock guardrail (tests/conftest.py): fails any unit test exceeding 8s total wall-clock time (setup + call + teardown), preventing future regressions from heavy fixture I/O

Results

Metric Before After
Full unit suite 10+ min 1:32
API tests only 2:51 (171s) 1:14 (74s)
test_setup.py 2:36 (156s), 3 crashes 23s, 0 crashes
Worker crashes 3 (gw nodes going down) 0
Slowest test setup 24s 1.7s

Test plan

  • Full unit suite: 12,269 passed, 5 skipped in 92s
  • Lint: all checks passed
  • Type-check: no issues in 1364 source files
  • Backup-specific tests: 21 passed (guard tests use mock service)

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
Copilot AI review requested due to automatic review settings April 2, 2026 20:14
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c656c8b7-eef5-4376-9944-c9f1e118bd3f

📥 Commits

Reviewing files that changed from the base of the PR and between db1d029 and 09e08a0.

📒 Files selected for processing (1)
  • tests/conftest.py

Walkthrough

Adds a pytest wall-clock guard in tests/conftest.py that records test start time and fails unit-marked tests exceeding an 8.0s threshold. Adds an autouse fixture in tests/unit/api/conftest.py that monkeypatches synthorg.api.app.build_backup_service to return None, preventing real BackupService construction during API unit tests. Adds an autouse fixture in tests/unit/api/controllers/test_backup.py that stubs a backup service with async mocks and empty list_backups, removing backup-related filesystem activity from backup controller tests.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: eliminate backup service I/O from API test lifecycle' accurately and concisely summarizes the main change—removing backup service filesystem I/O from API tests to fix performance regressions.
Description check ✅ Passed The description is comprehensive and clearly related to the changeset, explaining the performance problem, specific fixes applied, and measurable improvements achieved.
Docstring Coverage ✅ Passed Docstring coverage is 87.50% which is sufficient. The required threshold is 40.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 2, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 09e08a0.
Ensure 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 Files

None

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +46 to +47
markers = {m.name for m in item.iter_markers()}
if "unit" in markers and elapsed > _UNIT_TEST_WALL_CLOCK_LIMIT:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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:

coderabbitai[bot]
coderabbitai bot previously approved these changes Apr 2, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_service to return None for API unit tests, avoiding backup creation/pruning work during TestClient startup/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:
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
def pytest_runtest_teardown(item: pytest.Item) -> None:
def pytest_runtest_teardown(
item: pytest.Item, nextitem: pytest.Item | None
) -> None:

Copilot uses AI. Check for mistakes.
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
Copy link
Copy Markdown

codecov bot commented Apr 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.67%. Comparing base (43e0707) to head (09e08a0).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Aureliolo Aureliolo merged commit 08d9183 into main Apr 2, 2026
10 of 11 checks passed
@Aureliolo Aureliolo deleted the fix/test-performance-backup-teardown branch April 2, 2026 20:21
Aureliolo added a commit that referenced this pull request Apr 3, 2026
🤖 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants