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

refactor: ci/test improvements #1238

Merged
merged 65 commits into from
Jan 23, 2025
Merged

refactor: ci/test improvements #1238

merged 65 commits into from
Jan 23, 2025

Conversation

cylewitruk
Copy link
Member

@cylewitruk cylewitruk commented Jan 18, 2025

Description

Our build/CI times were starting to annoy me, so I took a stab at refactoring this a bit. Got a bit bigger than I thought it would, but this is the result of a fair bit of trial-and-error -- at least it deletes more than it adds 😄

A lot of the changed files are just moved or deleted files, or removing the #[cfg_attr(not(feature = "integration-tests"), ignore)] attribute.

This PR brings CI times down from ~10 minutes to ~4 minutes. This could probably be further reduced to closer to ~3m with #1223 (which reduces runtime of integration tests from ~ 3m to ~1m).

Changes

Workspace

  • Refactored the Makefile a fair bit; all of the targets should work as expected now (unless I missed to test something)
  • The Makefile now handles the 3x emily API clients (private, public, testing).
  • Removed the integration-tests feature:
    • This is unnecessary as unit tests can be selected by running with the --lib cargo/nextest flag, and integration tests with the --test integration argument.
    • This lets us avoid an extra build with the feature vs. not.
    • The integration tests and Makefile targets have been updated to reflect this.
  • All testing uses nextest now.
  • Consolidated some dependencies and their features into the workspace Cargo.toml.
    • Had to make some updates updates to blocklist-client and emily integration tests due to the config crate version/API differences.
  • Bumped rust to 1.81 so that make emily-as-lambda works.
    • Minor refactor in libp2p event_loop.rs due to new clippy lint.
  • Code generation:
    • We no longer try to generate code all the time (we have it checked in). make build or the respective specialized codegen targets will still do this if needed, though.
    • ...but we now have checks which will fail if the generated code is out-of-date vs. its generators.
    • Moved all generators out of .generated-sources and into the main workspace. Now only actually generated code lives there.
    • Made the openapi-generator-cli less noisy (error logs only).
    • Removed the clean commands for generators since we don't want to delete the generated sources anymore (checked in).

CI

  • Consolidated "on push" jobs to the on-push.yaml workflow.
    • This makes it easier with job dependencies.
    • I generally find it more coherent to keep related jobs from the same trigger in the same workflow (in this case on-push).
    • Yes, the jobs still run in parallel, and you still see the individual jobs in PRs (see the "Checks" at the bottom of this PR).
  • Bumped job runners to ubuntu-24.04.
  • Consolidates tool versions to environment variables at the top.
  • Use nextest archives to separate the building of test artifacts from the running of them. Now we run unit and integration jobs concurrently.
  • Use the mold linker -- generally faster than ld.
  • Optimize how integration tests are run:
    • We now work on starting the integration environment (takes about ~1m) in parallel to building the test artifacts so that the environment should be ready at just about the same time as the build artifacts (nextest archives) are ready, and once the environment is ready we wait for the artifacts (if needed). example
    • Removed the emily-aws-setup container in CI and run the initialize.py directly. The container build took unnecessary time.
    • Use nextest partitioning to split-up the integration tests over parallel jobs.
  • Added checks for stale generated code for blocklist and emily clients (in addition to contracts).
  • All project lints are consolidated into the lints job.
  • Added commit hashes for all used actions ([Feature]: Pin action by hash in CI workflows #1235).
  • Sets all default permissions for the workflow to read ([Feature]: Ensure GitHub workflows have only read permissions (unless needed) #1234).
  • Unit & integration tests now run with --retries 2, so if there are any flaky tests the run will have a chance at succeeding (note that flaky tests are reported in the logs).
  • Commented the workflow jobs.

Well I think that's the gist of it... Probably forgot something 😅

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@cylewitruk
Copy link
Member Author

I think this is OK and have given it a quick look given limited time available.

I have seen a lot of things I like -- my only wish would have been for this to be smaller and split into multiple PRs.

Yeah, it's bigger than I thought it would be, it was a lot of trial and error and things are sort-of intertwined.

Now that I have it buttoned up and know what works I could probably split it over multiple successive PR's, but that'll take time to do, so I opted to PR it as-is and move on to things in the backlog.

Copy link
Collaborator

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Thank you, 🙏🏽. I need to give some of these changes another look, but this largely looks good. I mainly just have a bunch of questions.

signer/src/bitcoin/fees.rs Outdated Show resolved Hide resolved
signer/tests/integration/bitcoin_client.rs Outdated Show resolved Hide resolved
signer/Cargo.toml Outdated Show resolved Hide resolved
docker/docker-compose.ci.yml Show resolved Hide resolved
blocklist-client/src/config.rs Show resolved Hide resolved
.github/workflows/on-push.yaml Outdated Show resolved Hide resolved
.github/workflows/on-push.yaml Outdated Show resolved Hide resolved
.github/workflows/on-push.yaml Show resolved Hide resolved
.github/workflows/on-push.yaml Outdated Show resolved Hide resolved
.github/workflows/on-push.yaml Outdated Show resolved Hide resolved
@cylewitruk
Copy link
Member Author

Added permissions: read-all to the on-push workflow in lieu of #1247 being merged

@cylewitruk
Copy link
Member Author

Added commit hashes for all actions in lieu of #1249 being PR'd

.github/workflows/on-push.yaml Show resolved Hide resolved
.github/workflows/on-push.yaml Outdated Show resolved Hide resolved
.github/workflows/on-push.yaml Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Collaborator

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Looks good. But I had only two things, one about building all targets and another about the retries, which I don't think get used in CI.

I did not take a close look at the Makefile changes for the emily client or the blocklist client so some extra eyes there would be nice.

blocklist-openapi-gen/package.json Outdated Show resolved Hide resolved
emily/openapi-gen/package.json Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
@cylewitruk cylewitruk merged commit 8b0dd85 into main Jan 23, 2025
7 checks passed
@cylewitruk cylewitruk deleted the feat/ci-improvements branch January 23, 2025 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants