Feat: split ci tests #1404
Feat: split ci tests #1404
Conversation
MarceloRobert
force-pushed
the
feat/split-ci-tests
branch
from
fd5e83a to
cf9dfc1
Compare
.github/workflows/ci.yaml
Outdated
| - name: Run tests | ||
| run: poetry run pytest --run-all | ||
| - name: Run integration tests | ||
| run: poetry run pytest -m "not unit" --run-all |
There was a problem hiding this comment.
Wouldn't it be better if we created a tag for integrations as well?
There was a problem hiding this comment.
Tag every test to follow a pattern, I mean
There was a problem hiding this comment.
Got it, I was able to make a "dynamic" marker where if the tests are in a "integration" or "unit" folders, they are automatically marked as that. And if they aren't marked as anything, an error will be raised
.github/workflows/ci.yaml
Outdated
| lint-and-unit-test-django: | ||
| if: github.event.pull_request.draft != true | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 |
There was a problem hiding this comment.
nit: 10 minutes for unit tests and lint seems to me an overworking. What do you think?
There was a problem hiding this comment.
Yeah, makes sense, I'll lower to 3 minutes. Sounds good?
There was a problem hiding this comment.
3/5 minutes sounds better. Nice!
MarceloRobert
force-pushed
the
feat/split-ci-tests
branch
2 times, most recently
from
d374535 to
3271e56
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR splits the CI pipeline to separate lint/format/unit tests from integration tests, enabling better identification of which test category failed. This addresses the issue where integration test failures (especially from forks) would mask linting and formatting failures.
- Adds pytest markers to distinguish unit tests from integration tests
- Creates a composite GitHub action for backend setup to reduce duplication
- Splits the single
build-djangojob into two separate jobs with appropriate timeouts
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| backend/pyproject.toml | Adds pytest markers for unit and integration test categorization |
| backend/conftest.py | Implements automatic test marking based on folder structure |
| .github/workflows/ci.yaml | Splits Django job into separate lint/unit and integration test jobs |
| .github/actions/setup-backend/action.yaml | Creates reusable composite action for backend setup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
backend/conftest.py
Outdated
| for marker in TEST_MARKERS: | ||
| if marker in parent_folder: | ||
| if len(added_markers) >= 1: | ||
| raise Exception( |
There was a problem hiding this comment.
Consider using a more specific exception type like ValueError or RuntimeError instead of the generic Exception class.
| raise Exception( | |
| raise ValueError( |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Not sure if ValueError would be the correct type of exception here
backend/conftest.py
Outdated
| added_markers.append(marker) | ||
|
|
||
| if not added_markers: | ||
| raise Exception("Test %s must have a type" % item.name) |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
MarceloRobert
force-pushed
the
feat/split-ci-tests
branch
from
3271e56 to
9d83b97
Compare
MarceloRobert
force-pushed
the
feat/split-ci-tests
branch
from
9d83b97 to
9ca1d05
Compare
backend/conftest.py
Outdated
| @@ -1,4 +1,36 @@ | |||
| from pytest import Item | |||
|
|
|||
| TEST_MARKERS = ["unit", "integration"] | |||
There was a problem hiding this comment.
Could we get the markers from pyproject.toml? It could be another script to get the markers
There was a problem hiding this comment.
Done. Makes sense to get it from there since I actually had to add the markers there otherwise I got warnings in the tests
MarceloRobert
force-pushed
the
feat/split-ci-tests
branch
from
9ca1d05 to
fc7edcb
Compare
MarceloRobert
force-pushed
the
feat/split-ci-tests
branch
from
fc7edcb to
20592d1
Compare
Description
Because the integrations tests and linting+formatting+unit tests were in a single job, we weren't able to identify which of them failed if just one of them did. This occurred specially when we expect the integration tests to fail in PRs from forks, but then we didn't see that the linting and formatting had also failed.
Changes
build-djangojob intolint-and-unit-tests-djangoandintegration-tests-django, both using the composite actionHow to test
Check the CI on this PR, and see that it passes
Closes #1402