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

feat(ci): add fully_synced_rpc_test test to CI #4223

Merged
merged 105 commits into from
May 5, 2022
Merged

Conversation

gustavovalverde
Copy link
Member

@gustavovalverde gustavovalverde commented Apr 27, 2022

Motivation

We want to add all completed lightwalletd integration tests to CI, to confirm we're not introducing bugs to the RPC endpoints or any other implementation related to lightwalletd

Fixes #4169
Fixes: #4305
Depends-On: #4252,#4266,#4271

Designs

  • Consider the implementations being done in test.yml and test-full-sync.yml

Solution

  • Add all available lightwalletd tests as different jobs to a single workflow called test-lightwalletd.yml
  • Add fully_synced_rpc_test test to CI
  • Add fully_synced_rpc_test to entrypoint.sh for easier execution

Review

@teor2345 or @dconnolly can review this

Reviewer Checklist

  • Verify tests are using the right execution methods
  • Confirm job ids, names and description
  • Validate if the last test should create a disk (as being done in the full sync test right now)

Follow Up Work

Simplify this job with the refactor being done for reusable workflows

@teor2345 teor2345 changed the base branch from main to lwd-sync-test April 27, 2022 21:42
@teor2345
Copy link
Contributor

I changed the base branch to PR #4177 to pick up the latest changes.

@gustavovalverde
Copy link
Member Author

I changed the base branch to PR #4177 to pick up the latest changes.

@teor2345 I might be changing it back to main if the tests result are not as expected, as tests won't run if the base branch is not targeting main (should we consider changing this behavior?). If everything runs as expected, we can leave it targeting lwd-sync-test

@gustavovalverde gustavovalverde self-assigned this Apr 27, 2022
@gustavovalverde gustavovalverde added A-infrastructure Area: Infrastructure changes A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement P-High 🔥 lightwalletd any work associated with lightwalletd labels Apr 27, 2022
@teor2345
Copy link
Contributor

I changed the base branch to PR #4177 to pick up the latest changes.

@teor2345 I might be changing it back to main if the tests result are not as expected, as tests won't run if the base branch is not targeting main (should we consider changing this behavior?). If everything runs as expected, we can leave it targeting lwd-sync-test

No worries - we just need to pick up the latest env var change. Feel free to rebase the branch, and change the GitHub base back.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Just a quick review to help with some of the Rust test details.

docker/entrypoint.sh Outdated Show resolved Hide resolved
.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
@gustavovalverde gustavovalverde marked this pull request as ready for review April 27, 2022 22:27
@gustavovalverde gustavovalverde requested a review from a team as a code owner April 27, 2022 22:27
@gustavovalverde gustavovalverde requested review from dconnolly and removed request for a team April 27, 2022 22:27
@gustavovalverde gustavovalverde requested a review from a team as a code owner April 28, 2022 03:20
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This branch needs to be rebased on the latest #4177 changes

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

It seems like the scope of this PR has grown a lot, so it might take a long time to get it merged.

In the test meeting, we agreed to:

  • start by putting the fully synced RPC test in CI
  • then put the lightwalletd full sync in CI, to generate the lightwalletd state for other tests
  • then get other devs in the team to work on CI PRs for the other integration tests

This PR seems to try to cover all the tests - can we split it up, and start with the fully synced RPC test?

.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
.github/workflows/test-lightwalletd.yml Outdated Show resolved Hide resolved
@gustavovalverde gustavovalverde requested a review from a team as a code owner April 29, 2022 02:07
@gustavovalverde gustavovalverde requested review from oxarbitrage and removed request for a team April 29, 2022 02:07
@gustavovalverde gustavovalverde changed the base branch from lwd-sync-test to main April 29, 2022 02:31
@gustavovalverde gustavovalverde changed the title feat(ci): add lightwalletd CI tests feat(ci): add lwd lightwalletd_full_sync and lightwalletd_update_sync CI tests Apr 29, 2022
@gustavovalverde gustavovalverde changed the title feat(ci): add lwd lightwalletd_full_sync and lightwalletd_update_sync CI tests feat(ci): add lightwalletd_*_sync CI tests Apr 29, 2022
@gustavovalverde gustavovalverde added the do-not-merge Tells Mergify not to merge this PR label Apr 29, 2022
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks, that looks a bit more manageable.

I think I got the order slightly wrong, I forgot we wanted to do Janito's tests next week.

So I think we might want to do a PR series like this:

  1. the fully synced RPC test in CI
  2. the send transaction test in CI
  3. the lightwalletd full sync in CI, to generate the lightwalletd state for other tests
  4. get other devs in the team to work on CI PRs for the other integration tests

What do you think? Have I missed anything else?

@teor2345 teor2345 dismissed their stale review April 29, 2022 04:33

We've cut down the scope now

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I didn't do a review, but there are some syntax errors in the lints.

.github/workflows/test.patch.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
@gustavovalverde gustavovalverde marked this pull request as draft May 4, 2022 21:41
@gustavovalverde gustavovalverde removed the do-not-merge Tells Mergify not to merge this PR label May 4, 2022
@gustavovalverde gustavovalverde marked this pull request as ready for review May 4, 2022 23:03
@gustavovalverde gustavovalverde requested a review from teor2345 May 4, 2022 23:41
@gustavovalverde
Copy link
Member Author

gustavovalverde commented May 5, 2022

Successful run: https://github.com/ZcashFoundation/zebra/actions/runs/2272744389

.github/workflows/test.yml Outdated Show resolved Hide resolved
Co-authored-by: teor <teor@riseup.net>
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks!

@teor2345
Copy link
Contributor

teor2345 commented May 5, 2022

The fully-synced-rpc test is not connected to the state:

May 05 01:34:27.563 WARN {net="Main"}:sync: zebrad::components::sync: could not download or verify genesis block, retrying error=DownloadFailed(Elapsed(()))

https://github.com/ZcashFoundation/zebra/runs/6299601576?check_suite_focus=true#step:8:265

We should not be seeing a genesis download in the logs.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

We need to connect the fully-synced-rpc test to the cached Zebra state

Copy link
Contributor

@teor2345 teor2345 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 ready to go, and independent of the cached tip state rebuild

mergify bot added a commit that referenced this pull request May 5, 2022
mergify bot added a commit that referenced this pull request May 5, 2022
@gustavovalverde gustavovalverde merged commit a23420b into main May 5, 2022
@gustavovalverde gustavovalverde deleted the lwd-ci-test branch May 5, 2022 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles A-infrastructure Area: Infrastructure changes C-enhancement Category: This is an improvement lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set RUST_LOG environmental variable in gcp-test-deploy.yml ci: call RPC test with fully synced Zebra
3 participants