-
-
Notifications
You must be signed in to change notification settings - Fork 476
ci: Split tests run in CI #12064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
ci: Split tests run in CI #12064
Conversation
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12064 +/- ##
==========================================
- Coverage 50.46% 50.20% -0.26%
==========================================
Files 90 90
Lines 23900 23900
Branches 5743 5743
==========================================
- Hits 12062 12000 -62
Misses 10357 10357
- Partials 1481 1543 +62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
.github/workflows/pull_request.yml
Outdated
download_tmp_dir: ${{ runner.temp }} | ||
- name: tests | ||
image: "openfoodfacts-server/backend:dev" | ||
# downloadbackendimage task loads the image into docker and keeps the original file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary for ishworkh/[email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really ? I will see docs and will update it thanks for pointing it out.
# Unit test groups for parallel execution in CI | ||
# Usage: make unit_test_group TEST_GROUP=1 | ||
# Groups are balanced by number of test files | ||
define UNIT_TEST_GROUPS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the groups be determined algorithmically? All jobs of the actions matrix use the same source, so that the groups could be built automatically to avoid having to add each test file here in the Makefile
individually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made group on there size as small are combined with large one so that run it in equal time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea of having hard-coded groups. It will be too easy to add a test and forget to include it which means the test will run locally but not in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point never though of this will look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JagjeevanAK I think it's better to create a script that you call and gives you back the test group, it should also take the number of group as parameter.
This script would list all tests, using file system) and separate into groups in a predictable manner (eg. after sorting the list), and print the list on stdout.
Then in makefile you can just call the script to get the test group.
The script might be written in python, as it's found on most machines today.
Please put the script in a sub folder, eg. scripts/tests/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, in bash this is as simple as eg.:
find tests/unit -name "*.t"|sort|split -n 1/10
find tests/unit -name "*.t"|sort|split -n 2/10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alex problem is that some test are taking time so I have mixed it with small and big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some e2e test like some what 2-3 if I am not wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the CI setup by adding grouped, parallel execution for both unit and integration tests in the Makefile and the GitHub Actions workflow.
- Introduce
unit_test_group
andintegration_test_group
targets in the Makefile to run test subsets in parallel. - Update
.github/workflows/pull_request.yml
to set up Docker Buildx, cache BuildKit, build the backend image, and use a matrix strategy for parallel test jobs.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
Makefile | Add grouped parallel test targets (unit_test_group , integration_test_group ) and supporting variables. |
.github/workflows/pull_request.yml | Configure Docker Buildx, caching layers, and matrix-based parallel unit/integration test jobs. |
Comments suppressed due to low confidence (3)
.github/workflows/pull_request.yml:80
- The
hashFiles
call includes single quotes around patterns, which can be interpreted literally. Remove the internal quotes so patterns are passed directly (e.g.,hashFiles(Dockerfile*, docker-compose.yml, docker/**, cpanfile*, conf/apache*)
).
key: ${{ runner.os }}-docker-${{ hashFiles('Dockerfile*', 'docker-compose.yml', 'docker/**', 'cpanfile*', 'conf/apache*') }}
.github/workflows/pull_request.yml:21
- [nitpick] The
filter
job is marked with a TODO and has no steps. If it’s not currently used, consider removing it to simplify the workflow.
filter:
Makefile:611
- [nitpick] The grouping logic spans many test files. Adding a brief explanation of the grouping strategy or moving the lists to a separate include file could improve readability and maintenance.
# Unit test groups for parallel execution in CI
my mistake while testing it remained Co-authored-by: Copilot <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
fix increased no. of groups form 4 to6 Co-authored-by: Copilot <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
Signed-off-by: Jagjeevan Kashid <[email protected]>
|
hey @alexgarel, This PR is ready to merge. |
It would be good to see some kind of comparison with not using groups but just running the unit and integration tests as separate parallel tasks. That way we can see how much time the group approach is actually saving |
group approach is good according to me because every time we need to spin the resource for each test and that might take time. |
# Unit test groups for parallel execution in CI | ||
# Usage: make unit_test_group TEST_GROUP=1 | ||
# Groups are balanced by number of test files | ||
define UNIT_TEST_GROUPS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JagjeevanAK I think it's better to create a script that you call and gives you back the test group, it should also take the number of group as parameter.
This script would list all tests, using file system) and separate into groups in a predictable manner (eg. after sorting the list), and print the list on stdout.
Then in makefile you can just call the script to get the test group.
The script might be written in python, as it's found on most machines today.
Please put the script in a sub folder, eg. scripts/tests/
mkdir -p tests/unit/outputs/ | ||
${DOCKER_COMPOSE_TEST} up -d memcached postgres mongodb | ||
@echo "🥫 Running all tests in group $(TEST_GROUP) with parallel execution and JUnit XML generation..." | ||
${DOCKER_COMPOSE_TEST} run ${COVER_OPTS} -e JUNIT_TEST_FILE="tests/unit/outputs/junit_group_$(TEST_GROUP).xml" -T --rm backend yath test --renderer=Formatter --renderer=JUnit --job-count=${CPU_COUNT} $(addprefix tests/unit/,$(call get_unit_group_tests,$(TEST_GROUP))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we loose coverage with this new run type ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to me it should not and this is ordered this way because each test was running double previously.
# Unit test groups for parallel execution in CI | ||
# Usage: make unit_test_group TEST_GROUP=1 | ||
# Groups are balanced by number of test files | ||
define UNIT_TEST_GROUPS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, in bash this is as simple as eg.:
find tests/unit -name "*.t"|sort|split -n 1/10
find tests/unit -name "*.t"|sort|split -n 2/10
@@ -289,10 +289,12 @@ unit_test: create_folders | |||
@echo "🥫 Running unit tests …" | |||
mkdir -p tests/unit/outputs/ | |||
${DOCKER_COMPOSE_TEST} up -d memcached postgres mongodb | |||
@echo "🥫 Running tests with yath (parallel jobs: ${CPU_COUNT})" | |||
${DOCKER_COMPOSE_TEST} run ${COVER_OPTS} -e JUNIT_TEST_FILE="tests/unit/outputs/junit.xml" -T --rm backend yath test --renderer=Formatter --renderer=JUnit --job-count=${CPU_COUNT} tests/unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here unit tests might run in parallel directly thanks to yath, without having to run them in different github runner. Is it that CPU_COUNT is too low in each runner ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to check
What
Screenshot
Related issue(s) and discussion