Speed up database test setup#825
Merged
Merged
Conversation
Build the schema once and clone it for isolated test databases. This keeps explicit migration coverage while avoiding hundreds of repeated Alembic runs. Confidence: high
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesTest infrastructure
Parallel coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confidence: high
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Keep the full ESGF regression on the main-only slow workflow. Run output contract tests against a representative catalog in pull request CI. Confidence: high
Run the pull request unit suite in four module scoped workers. Collect and combine coverage from each worker before reporting. Confidence: high
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun coverage reporting even if tests fail.
Currently, if any test step fails, the subsequent "Coverage report" step is skipped. This prevents the combination and upload of partial coverage data to Codecov, which can complicate debugging by missing coverage metrics on failing runs. Consider adding
if: always()orif: success() || failure()to these steps to ensure coverage artifacts are always generated and uploaded.
.github/workflows/ci.yaml#L65-L65: Addif: always()to the "Coverage report" step in thetests-corejob..github/workflows/ci.yaml#L103-L103: Addif: always()to the "Coverage report" step in thetests-integrationjob..github/workflows/ci.yaml#L148-L148: Addif: always()to the "Coverage report" step in thetests-providersjob.🛠️ Proposed fix for `.github/workflows/ci.yaml#L65-L65`
- name: Coverage report # Per-job coverage is partial (the integration job covers the rest), so # the aggregate fail_under gate is enforced by codecov/project rather # than here -- both jobs upload to the same `core` flag. + if: always() run: | uv run coverage combine
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a75136d5-c2e1-41fa-b252-7721d406bf35
📒 Files selected for processing (14)
.github/workflows/ci.yamlMakefilechangelog/825.improvement.mddocs/development.mdpackages/climate-ref/conftest.pypackages/climate-ref/src/climate_ref/conftest_plugin.pypackages/climate-ref/tests/unit/cli/test_db.pypackages/climate-ref/tests/unit/cli/test_root.pypackages/climate-ref/tests/unit/datasets/test_cmip_adapters.pypackages/climate-ref/tests/unit/datasets/test_datasets.pypackages/climate-ref/tests/unit/test_solve_helpers.pypackages/climate-ref/tests/unit/test_solve_regression.pypackages/climate-ref/tests/unit/test_testing.pypyproject.toml
Cloning into a per-test config directory that already exists raised FileExistsError from mkdir(parents=True). Pass exist_ok=True so a pre-existing target directory is fine. Also corrects the iter_local_datasets comment: CMIP6DatasetAdapter defines the method directly, and monkeypatch.delattr restores it after the test.
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
1 participant
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.
Description
Reuses a session scoped migrated SQLite template across isolated test databases. This avoids repeated Alembic migrations while keeping dedicated coverage for fresh database migrations.
Moves the full ESGF solver regression to the main slow suite. Pull request CI retains fast solver output-contract coverage against a representative catalog.
Runs
test-ref-unitin four xdist workers on public GitHub hosted Linux runners. Coverage records each worker and subprocess, then combines their data before reporting.Matched clean worktrees showed the template change reduced
test-ref-unitfrom 293.53 to 164.27 seconds. The slow-regression split further reduced the current branch from 181.16 to 136.31 seconds locally. Four local workers completed the current no-slow unit suite in 59.06 seconds.Checklist
Please confirm that this pull request has done the following:
changelog/Summary by CodeRabbit