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

test(rpc): Add Rust tests for lightwalletd sync from Zebra #4177

Merged
merged 21 commits into from
Apr 29, 2022
Merged

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Apr 22, 2022

Motivation

We want to run full sync and quick sync integration tests for lightwalletd.

We want to merge PR #4068 first, to unblock some other work.

Depends-On: #4068

CI Commands

You'll need to update the cache paths to match the CI paths.

To test full sync:

export ZEBRA_CACHED_STATE_PATH="$HOME/.cache/zebra"
export RUST_LOG="info"

cargo test -- --ignored --nocapture  lightwalletd_full_sync

To test quick update sync:

export ZEBRA_CACHED_STATE_PATH="$HOME/.cache/zebra"
export LIGHTWALLETD_DATA_DIR="$HOME/.cache/lightwalletd"
export RUST_LOG="info"

cargo test -- --ignored --nocapture  lightwalletd_update_sync

Manual Testing

To manually test all the lightwalletd integration tests in series (which avoids state conflicts), use:

export ZEBRA_CACHED_STATE_PATH="$HOME/.cache/zebra"
export LIGHTWALLETD_DATA_DIR="$HOME/.cache/lightwalletd"
export RUST_LOG="info"

cargo test -- --ignored --nocapture  lightwalletd_test_suite

Solution

  • Create a lightwalletd full sync test
  • Create a lightwalletd cached state update test

Details:

  • Create a LightwalletdTestType enum
  • Make the lightwalletd integration test take a test type
  • Configure lightwalletd tests based on the test type
  • Add checks for each LightwalletdTestType
  • Create test functions that run each lightwalletd test type
  • Fail if the state doesn't match the test type

Related Cleanups:

  • Remove obsolete kill_on_error() in the lightwalletd test
  • Add a sub-directory for lightwalletd test state
  • Move failure messages into their own module

Closes #3511
Closes #4166

Review

@jvff is probably be the best person to review this PR.

@gustavovalverde this PR should work in CI if you use the commands above.
(It's draft because I haven't finished testing it yet.)

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

@teor2345 teor2345 added P-High 🔥 C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces lightwalletd any work associated with lightwalletd labels Apr 22, 2022
@teor2345 teor2345 requested a review from jvff April 22, 2022 04:37
@teor2345 teor2345 self-assigned this Apr 22, 2022
@teor2345 teor2345 marked this pull request as ready for review April 25, 2022 00:29
@teor2345 teor2345 requested a review from a team as a code owner April 25, 2022 00:29
@teor2345 teor2345 changed the title test(rpc): Add lightwalletd sync integration test harnesses test(rpc): Add Rust tests for lightwalletd sync from Zebra Apr 25, 2022
@teor2345
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 26, 2022

update

✅ Branch has been successfully updated

jvff
jvff previously approved these changes Apr 27, 2022
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Looks great! There's just one minor tweak, to avoid having duplicate environment variables with different names after resolving any merge conflicts.

zebrad/tests/common/config.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 requested review from jvff and removed request for a team April 27, 2022 21:29
@teor2345
Copy link
Contributor Author

When rebasing on main, I removed a bunch of duplicate code.

The LightwalletdTestType enum names are still a bit messy, but I'd like to fix that later.

@teor2345
Copy link
Contributor Author

Failed due to #4234:

Error: buildx failed with: error: failed to solve: failed to fetch oauth token: unexpected status: 401 Unauthorized

@oxarbitrage oxarbitrage self-requested a review April 29, 2022 18:13
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I think it is ok to merge this now as it will conflict with some other code i am working on. The comment from @jvff was addressed and it also looks good to me.

mergify bot added a commit that referenced this pull request Apr 29, 2022
@mergify mergify bot merged commit 59bdab1 into main Apr 29, 2022
@mergify mergify bot deleted the lwd-sync-test branch April 29, 2022 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces C-testing Category: These are tests lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add lightwalletd quick tip sync test Add lightwalletd full sync integration test
3 participants