Skip to content
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

chore: Parallelize CircleCI lint build #341

Merged
merged 5 commits into from
Apr 3, 2023
Merged

chore: Parallelize CircleCI lint build #341

merged 5 commits into from
Apr 3, 2023

Conversation

vitsalis
Copy link
Member

@vitsalis vitsalis commented Mar 28, 2023

The sequential execution of all of the CircleCI steps takes too long. In this PR we parallelize them to improve CI times.

@vitsalis vitsalis changed the title chore: Parallelize CircleCI builds chore: Parallelize CircleCI lint build Mar 28, 2023
@vitsalis
Copy link
Member Author

vitsalis commented Mar 28, 2023

Didn't see much of an improvement on CI times when I tried parallelizing the test and build jobs individually. Probably due to the resources being stretched across the jobs. Further, for some reason the integration tests failed (might be worth checking out the log @KonradStaniec). For that, I made the PR to only parallelize the lint job.

@SebastianElvis
Copy link
Member

Didn't see much of an improvement on CI times when I tried parallelizing the test and build jobs individually. Probably due to the resources being stretched across the jobs.

Another possible reason might be that the build-test step (which builds Babylon and executes all tests sequentially) dominates the execution time. From my experience lint takes negligible time (<1min) within a CI execution.

Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

build-test:
jobs:
- build-test
lint:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that lint takes little time and its result does not depend on build-test, maybe putting lint before build-test is better?

Copy link
Member Author

@vitsalis vitsalis Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are running in parallel so it doesn't matter which one we put first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok you are right. All good then

Copy link
Collaborator

@KonradStaniec KonradStaniec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further, for some reason the integration tests failed (might be worth checking out the log @KonradStaniec). For that, I made the PR to only parallelize the lint job.

Yea some of integration tests needs improvements in terms of being deterministc. As we are after whole update cycle in terms of cosmos libraries this probably good moment to do that.

In general it maybe worth doing e2e tests and integration tests in parallel after building docker image ?

One thing that I am also considering is to to remove integration tests and leaving only e2e tests, as this e2e tests are much more powerfull in terms of configuration. After adding few more scenarios, they could test the same things as integration tests. This would definitly speed up CI.

lint:
machine:
image: ubuntu-2204:2022.10.1
resource_class: large
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it makes sense to use smaller image just for linting ? (like medium)

@vitsalis
Copy link
Member Author

vitsalis commented Apr 3, 2023

In general it maybe worth doing e2e tests and integration tests in parallel after building docker image ?

Not entirely sure how we can accomplish that as iirc parallel jobs work on their own environment. Eitherway, the e2e tests do not need a Docker image right? I tried parallelizing the jobs before but I suffer the determinism issues outlined previously

One thing that I am also considering is to to remove integration tests and leaving only e2e tests, as this e2e tests are much more powerfull in terms of configuration. After adding few more scenarios, they could test the same things as integration tests. This would definitly speed up CI.

This sounds like a good idea. Currently, the integration tests are very slow given that the localnet needs to come up before they are executed.

@KonradStaniec
Copy link
Collaborator

Eitherway, the e2e tests do not need a Docker image right? I tried parallelizing the jobs before but I suffer the determinism issues outlined previously

They actually do, but they are running after integration tests and integration tests build a babylon docker image, and as both integration tests and e2e tests run in the same enviroments the image is available for integration tests.

@vitsalis
Copy link
Member Author

vitsalis commented Apr 3, 2023

Eitherway, the e2e tests do not need a Docker image right? I tried parallelizing the jobs before but I suffer the determinism issues outlined previously

They actually do, but they are running after integration tests and integration tests build a babylon docker image, and as both integration tests and e2e tests run in the same enviroments the image is available for integration tests.

I see. Let's find a way to speed integration + e2e as it is very difficult to test right now using them, especially if a build fails. Otherwise, let's try to make the integration tests more deterministic as most of the failures happen due to non-determinism.

@vitsalis vitsalis merged commit c05c04c into dev Apr 3, 2023
@vitsalis vitsalis deleted the parallel-builds branch April 3, 2023 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants