ci: skip integration tests on Windows to reduce CI timeout #779
Conversation
- Add test-unit-tests poe task for running only unit tests - Modify CI workflow to run only unit tests on Windows - Keep all tests running on Ubuntu for full coverage - This should reduce Windows CI time from 1hr+ to ~22min like Ubuntu Co-Authored-By: AJ Steers <[email protected]>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1757450012-skip-windows-integration-tests' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1757450012-skip-windows-integration-tests'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
- Revert job name to original format per PR feedback - Create two separate pytest steps with opposite conditions - 'Run Pytest [all]' for Ubuntu (matrix.os != 'Windows') - 'Run Pytest [unit tests only]' for Windows (matrix.os == 'Windows') - Addresses GitHub comment about job names being hard-coded in merge requirements Co-Authored-By: AJ Steers <[email protected]>
📝 WalkthroughWalkthroughSplit CI pytest step by OS: non-Windows runs the original pytest invocation, Windows runs pytest limited to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Commit/PR
participant GH as GitHub Actions
participant Runner as Matrix Runner
participant PyTest as pytest
Dev->>GH: Trigger workflow
GH->>Runner: Start job (matrix.os, python-version)
note right of Runner #DDEBF7: Job name and step gated by OS
alt matrix.os == "Windows"
Runner->>PyTest: pytest tests/unit_tests/
else matrix.os != "Windows"
Runner->>PyTest: pytest (original selection)
end
PyTest-->>Runner: Exit code
Runner-->>GH: Report result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Would you like me to suggest consolidating step names or adding a brief workflow comment explaining the OS split, wdyt? 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/python_pytest.yml (1)
146-229: Harden GITHUB_TOKEN permissions for this workflow.CodeQL’s suggestion is solid: can we explicitly set permissions to least privilege, e.g., contents: read, wdyt?
name: Run Tests on: push: @@ pull_request: {} +permissions: + contents: read + env: AIRBYTE_ANALYTICS_ID: ${{ vars.AIRBYTE_ANALYTICS_ID }}
🧹 Nitpick comments (3)
pyproject.toml (1)
186-186: Add a coverage-backed poe task and (optionally) reuse it in CI?Nice addition. To keep the “unit-tests-only” target in one place and avoid repeating the path logic in CI, would you like to add a coverage variant and/or have Windows use the poe task, wdyt?
[tool.poe.tasks] test = { shell = "pytest" } test-fast = { shell = "pytest --durations=5 --exitfirst -m 'not slow'" } test-unit-tests = { shell = "pytest tests/unit_tests/" } +test-unit-tests-coverage = { shell = "coverage run -m pytest tests/unit_tests/" }If you want this discoverable by any internal CI helpers, consider appending it to the airbyte_ci list too:
[tool.airbyte_ci] extra_poetry_groups = ["dev"] -poe_tasks = ["test"] +poe_tasks = ["test", "test-unit-tests"].github/workflows/python_pytest.yml (2)
146-146: Dynamic job name reads fine; prefer a matrix flag to avoid duplicating expressions?The inline ternary works, but you’re now repeating the same condition here and in the pytest command. Would you consider introducing a matrix field (e.g., subset: unit|all) and referencing it in both places for clarity, wdyt?
strategy: matrix: - python-version: [ + python-version: [ '3.10', '3.11', ] - os: [ - Ubuntu, - Windows, - ] + os: [Ubuntu, Windows] + include: + - os: Windows + subset: Unit Tests Only + test_path: tests/unit_tests/ + - os: Ubuntu + subset: All Tests + test_path: "" fail-fast: false @@ - name: Pytest (${{ matrix.os == 'Windows' && 'Unit Tests Only' || 'All Tests' }}, Python ${{ matrix.python-version }}, ${{ matrix.os }}) + name: Pytest (${{ matrix.subset }}, Python ${{ matrix.python-version }}, ${{ matrix.os }})
190-194: Switch to the poe task (or a matrix-provided path) for Windows to centralize test selection?To keep logic DRY and aligned with pyproject, would you like to drive the path via matrix (see prior comment) or call the poe task on Windows, wdyt?
Option A (use matrix.test_path from prior diff):
- poetry run coverage run -m pytest - --verbose - ${{ matrix.os == 'Windows' && 'tests/unit_tests/' || '' }} + poetry run coverage run -m pytest --verbose ${{ matrix.test_path }} -m "not linting and not super_slow and not flaky"Option B (split steps and reuse poe task):
- - name: Run Pytest + - name: Run Pytest (Windows - Unit) + if: matrix.os == 'Windows' timeout-minutes: 60 env: GCP_GSM_CREDENTIALS: ${{ secrets.GCP_GSM_CREDENTIALS }} - run: > - poetry run coverage run -m pytest - --verbose - ${{ matrix.os == 'Windows' && 'tests/unit_tests/' || '' }} - -m "not linting and not super_slow and not flaky" + run: > + poetry run coverage run -m poe test-unit-tests + -m "not linting and not super_slow and not flaky" + + - name: Run Pytest (Ubuntu - Full) + if: matrix.os == 'Ubuntu' + timeout-minutes: 60 + env: + GCP_GSM_CREDENTIALS: ${{ secrets.GCP_GSM_CREDENTIALS }} + run: > + poetry run coverage run -m pytest --verbose + -m "not linting and not super_slow and not flaky"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/python_pytest.yml(2 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/python_pytest.yml
[warning] 146-229: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (Unit Tests Only, Python 3.11, Windows)
- GitHub Check: Pytest (Unit Tests Only, Python 3.10, Windows)
- GitHub Check: Pytest (All Tests, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All Tests, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (1)
.github/workflows/python_pytest.yml (1)
164-176: Approve runner labels and tests setupUbuntu-latest and Windows-latest resolve correctly on Actions, and
tests/unit_testscontains 82 tests—can we remove the extra verification script? wdyt?
aaronsteers
deleted the
devin/1757450012-skip-windows-integration-tests
branch
1 participant
ci: skip integration tests on Windows to reduce CI timeout
Summary
Addresses the Windows CI timeout issue where tests were taking 1hr+ vs 22min on Ubuntu. This PR implements a two-part solution:
test-unit-teststhat runs onlypytest tests/unit_tests/(182 unit tests)The conditional logic uses
${{ matrix.os == 'Windows' && 'tests/unit_tests/' || '' }}to add the directory restriction only for Windows runners. Ubuntu continues running all tests for full coverage.Review & Testing Checklist for Human
Testing the changes locally:
Notes
Link to Devin run: https://app.devin.ai/sessions/2cd62f25e51440258413a76d0e0763c6
Summary by CodeRabbit
New Features
poe test-unit-tests.Tests
Chores
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.