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

dynamic circleci config for streamlining test execution #6561

Merged
merged 11 commits into from
Jun 23, 2021

Conversation

raulk
Copy link
Member

@raulk raulk commented Jun 22, 2021

This PR introduces dynamic CircleCI config generation to speed up our CI. Concretely, it generates scoped integration and unit test jobs, all of which run in parallel.

An integration test job is generated for every file in itest. We use the go test <file> invocation form, which has the limitation that itests cannot reference other symbols in the same package (they can of course reference symbols from other packages, like itests/kit), but that's a fair tradeoff because itests are supposed to be self-contained anyway.

Unit test jobs are generated as per configuration. I tried to respect the old grouping of suites. All other top-level directories that are NOT grouped under a suite get run under the rest suite.

I explored using CircleCI dynamic configuration, which enables you to generate the CircleCI config on their CI, and then re-submit it for execution. I found this introduced a head-of-line latency of around 1min, which I didn't like. So I removed it in favour of a make target.

Therefore, developers are responsible for regenerating the CircleCI config on their workstations, using make circleci. They should do this every time they add a new itest, or a new top-level package with unit tests.


⚠️ TODO

  • In the future, we may want to add a check that runs the make target on CI, and fails the workflow if the diff is non-empty, similar to what we do with go mod tidy.

To parallelise things further, all you need to do is break up itest files containing many/slow tests into more itest files. This is for integration tests. For unit tests, you can create new groups.


I've removed the dependency between the build jobs and the test-short job. Didn't really make much sense.

@raulk raulk changed the title dynamic circleci config. dynamic circleci config for streamlining test execution Jun 22, 2021
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

This is great, just 1 comment

itests/sector_terminate_test.go Outdated Show resolved Hide resolved
@magik6k
Copy link
Contributor

magik6k commented Jun 22, 2021

Therefore, developers are responsible for regenerating the CircleCI config on their workstations, using make circleci. They should do this every time they add a new itest, or a new top-level package with unit tests.

The nice thing is that make gen runs go generate ./..., and the gen-check CI step will fail when one won't do that

@BigLep
Copy link
Member

BigLep commented Jun 23, 2021

@raulk : this is awesome.

In the future, we may want to add a check that runs the make target on CI, and fails the workflow if the diff is non-empty, similar to what we do with go mod tidy.

If I'm understanding this right, I think we need to do this now. I don't think we should check anything into the codebase the relies on "best intentions" to get right.

@raulk
Copy link
Member Author

raulk commented Jun 23, 2021

@BigLep agree with your reasoning. Based on what @magik6k said above, this is already taken into account by running go generate ./..., and then checking for diffs and failing the CI task if diffs are present. I just pushed a commit to check if it works as intended, because I suspect that go generate ./... might ignore hidden directories (starting with a dot, like the .circleci dir is).

@raulk
Copy link
Member Author

raulk commented Jun 23, 2021

Confirmed, it doesn't work as expected. I'll modify the gen-check CI job to specifically go generate the hidden directory too, as it's not picked up by the ./... expression.

image

@raulk
Copy link
Member Author

raulk commented Jun 23, 2021

Bingo! 0c31676 (#6561) did the trick.

image

@BigLep @magik6k the TODOs and review comments are all addressed.

@raulk
Copy link
Member Author

raulk commented Jun 23, 2021

@magik6k work here is done.

@magik6k magik6k merged commit 643b35c into master Jun 23, 2021
@magik6k magik6k deleted the raulk/circleci-dynamic branch June 23, 2021 17:17
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