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

split CI tests #24

Closed
wants to merge 19 commits into from
Closed

split CI tests #24

wants to merge 19 commits into from

Conversation

miraclx
Copy link
Contributor

@miraclx miraclx commented Aug 17, 2022

Current CI runs use upwards of an hour to complete. — https://github.com/near/cargo-near/actions/runs/2872416612

This is an attempt at cutting that time by using multiple nodes to run the tests in parallel.

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

neat trick to split tests. Unfortunate how many distinct tests there are with the 4 combinations of 5 tests, though

.github/workflows/test.yml Outdated Show resolved Hide resolved
args: --ignore RUSTSEC-2020-0071

- name: Run Audit Tool
run: cargo audit --ignore RUSTSEC-2020-0071
Copy link
Contributor

Choose a reason for hiding this comment

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

note to self or for someone else to remember, if we remove the SDK dependency in #23, we can remove this ignore

@miraclx
Copy link
Contributor Author

miraclx commented Aug 17, 2022

Turns out, GitHub has a hard limit on how many jobs run in parallel at any point in time.

It's 20 in total and a maximum of 5 of them for macOS.

https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits

And from the tests, it seems to prioritize running Ubuntu over macOS. Perhaps I should switch the matrix order.

@austinabell
Copy link
Contributor

Turns out, GitHub has a hard limit on how many jobs run in parallel at any point in time.

It's 20 in total and a maximum of 5 of them for macOS.

docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits

And from the tests, it seems to prioritize running Ubuntu over macOS. Perhaps I should switch the matrix order.

Or maybe just split into fewer groups? There are 6 non-batch options, so if it was split into 4 groups, it would be one over that 20 total? no opinions here

@miraclx
Copy link
Contributor Author

miraclx commented Aug 17, 2022

It's 20 in total and a maximum of 5 of them for macOS.

Looking at https://github.com/orgs/community/discussions/26343#discussioncomment-3251562, turns out that limit is organization-wide.

So all repositories under the near org have only 5 macOS runners to share.

This affects us quite a bit, as the current PR state requires 6 of those runners.

And looking at the last CI run – https://github.com/near/cargo-near/actions/runs/2877970694/usage

The macOS jobs seem to have waited on each other. Increasing our CI runtime even more than originally.

For this reason, I'm just going to close this PR.

I wonder if it's worth it to explore using the same node to test with the stable and 1.56.0 toolchains.

Quick question though? why are we testing 1.56.0? is that our MSRV?

@miraclx miraclx closed this Aug 17, 2022
@itegulov
Copy link
Contributor

Quick question though? why are we testing 1.56.0? is that our MSRV?

That is indeed MSRV, but I wonder if MSRV even makes sense for a binary...

Another thing to consider: maybe we should only run integration tests as a cron job (e.g. once per day) and ping us all if it fails?

@miraclx
Copy link
Contributor Author

miraclx commented Aug 18, 2022

That is indeed MSRV, but I wonder if MSRV even makes sense for a binary...

Not really. But even if it did, we wouldn't need to run tests for MSRV. A simple cargo check should suffice.

maybe we should only run integration tests as a cron job (e.g. once per day) and ping us all if it fails?

Yeah, we can do that. Although, would there ever be a case where the tests would yield different results between runs? Since all the variables that go into the tests are constant. I guess the only case would be dependency version changes.

@itegulov
Copy link
Contributor

Not really. But even if it did, we wouldn't need to run tests for MSRV. A simple cargo check should suffice.

Ok, let's just get rid of 1.56.0 then. Probably not worth even doing cargo check.

Yeah, we can do that. Although, would there ever be a case where the tests would yield different results between runs? Since all the variables that go into the tests are constant. I guess the only case would be dependency version changes.

I guess this would only be useful if this repo had >1 PRs merged per day, so yeah probably does not make sense for now. Only running integration tests on master (i.e. after you merge a PR) would be a good middle ground though I think.

@miraclx miraclx mentioned this pull request Aug 18, 2022
@austinabell
Copy link
Contributor

That is indeed MSRV, but I wonder if MSRV even makes sense for a binary...

It only matters such that if you run cargo install, you need to have that toolchain installed.

@miraclx
Copy link
Contributor Author

miraclx commented Aug 18, 2022

It only matters such that if you run cargo install, you need to have that toolchain installed.

For that, we can use the rust-version field in Cargo.toml.

@austinabell
Copy link
Contributor

For that, we can use the rust-version field in Cargo.toml.

Well, that indicates which version is required. The reason I'm saying MSRV is useful in CI is that you don't introduce changes that unnecessarily increase MSRV, requiring devs to upgrade/install newer toolchain to install. Nbd though

@miraclx
Copy link
Contributor Author

miraclx commented Aug 18, 2022

You're right. We can reintroduce it, but leave it at cargo check instead of running the full test suite. - #30

@miraclx miraclx mentioned this pull request Aug 18, 2022
@miraclx miraclx deleted the split-ci-tests branch August 29, 2022 08:50
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