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

[build] Run Venice CI with GitHub Actions #115

Merged
merged 13 commits into from Dec 23, 2022

Conversation

sushantmane
Copy link
Contributor

Run Venice tests on the following matrix:
OS: macOS, Ubuntu
JDK: 11

Run Venice CI with GitHub Actions

While we figure out how we want to run CI with GitHub actions, I think it would be good to have
something up and running in the meantime. With this RB, CI will be run on every commit/pull request.
It triggers one job/OS for unit tests (~42 mins) and one for integration tests/OS (2.30 hours and a bit flaky).

How was this PR tested?

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to explain your proposed changes and call out the behavior change.

@FelixGV
Copy link
Contributor

FelixGV commented Nov 21, 2022

Thanks Sushant! Is it normal that the checks show as failed?

@sushantmane
Copy link
Contributor Author

sushantmane commented Nov 21, 2022

Thanks Sushant! Is it normal that the checks show as failed?

No, checks should not fail. I know the reason: this PR ('get fetch' part in actions) was designed to work on forks and I need to update it to work everywhere. I'll update it. Thanks.

@sushantmane sushantmane force-pushed the ci-with-github-actions branch 2 times, most recently from 15d17f9 to 61be3a7 Compare December 5, 2022 18:55
@FelixGV
Copy link
Contributor

FelixGV commented Dec 15, 2022

Hi Sushant, would you mind rebasing and retrying this? Curious to see how it goes now. Thanks!

@FelixGV
Copy link
Contributor

FelixGV commented Dec 20, 2022

I think we need to divide the e2e tests further. One first cut might be to do a split along the lines of:

  1. Run all integration tests that match com.linkedin.venice.endToEnd.*.
  2. Run all integration tests that don't match com.linkedin.venice.endToEnd.*

Might need to define a couple more test targets in the Gradle build. This seems useful for that: https://mkyong.com/gradle/gradle-how-to-exclude-some-tests/

Are you game to give it a try?

@sushantmane
Copy link
Contributor Author

I think we need to divide the e2e tests further. One first cut might be to do a split along the lines of:

  1. Run all integration tests that match com.linkedin.venice.endToEnd.*.
  2. Run all integration tests that don't match com.linkedin.venice.endToEnd.*

Might need to define a couple more test targets in the Gradle build. This seems useful for that: https://mkyong.com/gradle/gradle-how-to-exclude-some-tests/

Are you game to give it a try?

Sorry for the late response. Sure, I'll split e2e tests into two parts. They used to finish in < 3HR and hence I added 3HR timeout. But it seems we've added >25 e2e tests since then.

@sushantmane
Copy link
Contributor Author

@FelixGV -

  1. Run all integration tests that match com.linkedin.venice.endToEnd.*.

Completes in 2h10m

  1. Run all integration tests that don't match com.linkedin.venice.endToEnd.*

Completes in 1h

@adamxchen is planning to work on this in the next quarter. In the meantime, should we use this PR?

@FelixGV
Copy link
Contributor

FelixGV commented Dec 21, 2022

Thanks a lot for the initiative Sushant!! I think we should merge this in, for sure!

In the future, I would like us to reduce the wall clock time by splitting further, and/or deleting redundant tests.

In any case, this is a great start! Thanks again 🙏

FelixGV
FelixGV previously approved these changes Dec 21, 2022
Copy link
Contributor

@FelixGV FelixGV left a comment

Choose a reason for hiding this comment

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

Tremendous 🤩🍾🙏

@sushantmane
Copy link
Contributor Author

Thanks for the review. I'll run our internal CI once just to make sure everything else is alright.

@sushantmane
Copy link
Contributor Author

@FelixGV - Could you please approve the PR again?

Copy link
Contributor

@nisargthakkar nisargthakkar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@FelixGV FelixGV left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Please make sure to review and clean up the commit message while squashing.

@sushantmane
Copy link
Contributor Author

Thank you @FelixGV & @nisargthakkar!

@sushantmane sushantmane merged commit 577c2ed into linkedin:main Dec 23, 2022
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.

None yet

3 participants