-
Notifications
You must be signed in to change notification settings - Fork 124
Added a workflow to parallelise the E2E tests. Updated E2E tests to c… #697
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?
Conversation
…reate new table names for each run to avoid issue in parallelisation
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
key: venv-${{ runner.os }}-${{ steps.setup-python.outputs.python-version }}-${{ github.event.repository.name }}-${{ hashFiles('**/poetry.lock') }} | ||
|
||
- name: Install dependencies | ||
if: steps.cached-poetry-dependencies.outputs.cache-hit != 'true' | ||
run: poetry install --no-interaction --no-root | ||
|
||
- name: Install library | ||
run: poetry install --no-interaction --all-extras | ||
|
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.
why is the poetry setup part being done multiple times, each part basically has the same setup
|
||
- name: Set up python | ||
id: setup-python | ||
uses: actions/setup-python@v5 | ||
with: | ||
python-version: "3.10" | ||
|
||
- name: Install Poetry | ||
uses: snok/install-poetry@v1 | ||
with: | ||
virtualenvs-create: true | ||
virtualenvs-in-project: true | ||
installer-parallel: true | ||
|
||
- name: Load cached venv | ||
id: cached-poetry-dependencies | ||
uses: actions/cache@v4 | ||
with: | ||
path: .venv | ||
key: venv-${{ runner.os }}-${{ steps.setup-python.outputs.python-version }}-${{ github.event.repository.name }}-${{ hashFiles('**/poetry.lock') }} |
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.
Again we are creating multiple environments , which is just too much overhead.
Simpler would be create a first job called setup that installs the poetry and activates the venv
Then find all the test files and execute them in matrix fashion within the setup done first
Don't think it is efficient to create the entire venv setup for each test
if: ${{ needs.discover-tests.outputs.common-test-files != '[]' }} | ||
strategy: | ||
matrix: | ||
test_file: ${{ fromJson(needs.discover-tests.outputs.common-test-files) }} |
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.
why are you running each single test is parallel. This is just too much overhead. When using a matrix you are basically spinning up a VM for each test and this is just wrong
The current setup has a lot of boiler plate and is just not efficient
My suggestion is to have jobs that are concentrated
|
I can see the tests still took 1 hr to run, so what exactly is the optimisation?
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
table_name = self.inline_table_name | ||
INSERT_QUERY = f"INSERT INTO {table_name} (`{target_column}`) VALUES (%(p)s)" | ||
SELECT_QUERY = f"SELECT {target_column} `col` FROM {table_name} LIMIT 1" | ||
DELETE_QUERY = f"DELETE FROM {table_name}" |
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 is a incomplete resource cleanup here. If the Insert or Select query fails then the Delete query will never be executed and so for every new run we will end up creating new garbage tables
@@ -0,0 +1,136 @@ | |||
name: Code Coverage | |||
|
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.
If this is satisfying the usecase, why have you not deleted this file -.github/workflows/coverage-check.yml
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.
Will remove both coverage-check and integrations workflow since the new workflow satisfies the needs. Just kept the workflow to ensure the coverage percentage is same for the new workflow as well.
@msrathore-db If you have modified the code such that tests can be run in parallel, why don't you updated the integration.yml test running also to use pytest xdist. So that integration test can also run faster |
Would be removing the workflow to avoid running e2e tests twice. |
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
What type of PR is this?
Description
Added a workflow to parallelise the E2E tests. Updated E2E tests to create new table names for each run to avoid issue in parallelisation
How is this tested?
Related Tickets & Documents