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

fix(tests): add submitblock test to CI, and avoid copying the cached state directory in other tests #5589

Merged
merged 17 commits into from
Nov 10, 2022

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Nov 8, 2022

Motivation

The send transactions and submitblock tests were needlessly copying the entire zebra cached state directory.

The submitblock test also included duplicate code, was missing some details, and hadn't been added to our CI.

There are a few commits that are purely code movement and renaming.

Solution

  • Renames LightwalletdTestType to TestType, adds a UpdateZebraCachedState variant, and moves it to a new file
  • Moves random_known_rpc_port_config fn to config.rs file
  • Replaces duplicate logic with methods from TestType and adds a needs_zebra_rpc_server method for determining the config
  • Adds get_raw_future_blocks and get_future_blocks fns for fully syncing Zebra and getting up to the first max_num_blocks in best chain of the non-finalized state
  • Uses the new fns for getting blocks up to the first max_num_blocks past the cached state in the send transaction and submitblock tests
  • Adds submitblock test to CI

Review

This PR is part of regular scheduled work.

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
  • How do you know it works? Does it have tests?

Follow Up Work

  • Remove main branch constraint on send transactions test in CI

@arya2 arya2 added C-bug Category: This is a bug C-enhancement Category: This is an improvement P-Medium ⚡ I-slow Problems with performance or responsiveness C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces labels Nov 8, 2022
@arya2 arya2 requested review from a team as code owners November 8, 2022 23:32
@arya2 arya2 requested review from teor2345 and removed request for a team November 8, 2022 23:32
@arya2
Copy link
Contributor Author

arya2 commented Nov 8, 2022

Running a send transactions test here: https://github.com/ZcashFoundation/zebra/actions/runs/3424052393

@arya2 arya2 self-assigned this Nov 8, 2022
@github-actions github-actions bot added C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Nov 8, 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, looks good!

I like the way you have re-used a lot of the existing code.

I have a few minor suggestions, and I noticed some things from the old tests that are missing.

.github/workflows/continous-integration-docker.patch.yml Outdated Show resolved Hide resolved
docker/entrypoint.sh Outdated Show resolved Hide resolved
zebrad/tests/common/cached_state.rs Show resolved Hide resolved
zebrad/tests/common/cached_state.rs Outdated Show resolved Hide resolved
zebrad/tests/common/test_type.rs Outdated Show resolved Hide resolved
zebrad/tests/common/lightwalletd/send_transaction_test.rs Outdated Show resolved Hide resolved
zebrad/tests/common/test_type.rs Show resolved Hide resolved
.github/workflows/continous-integration-docker.yml Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor

teor2345 commented Nov 9, 2022

Note for other reviewers:

It was very tricky for me to review the changes to zebrad/tests/common/test_type.rs, because the code was modified and moved in the same PR. GitHub and git diffs don't really deal with that very well.

Here's how I got a diff:

git checkout origin/update-ci-and-long-running-tests zebrad/tests/common/test_type.rs
cd ..
git diff --unified=10 zebra/zebrad/tests/common/lightwalletd.rs zebra/zebrad/tests/common/test_type.rs

@teor2345 teor2345 changed the title fix(tests) avoids copying the cached state directory and adds submitblock test to CI fix(tests): add submitblock test to CI, and avoid copying the cached state directory in other tests Nov 9, 2022
@teor2345 teor2345 removed the C-feature Category: New features label Nov 9, 2022
@teor2345
Copy link
Contributor

teor2345 commented Nov 9, 2022

If this PR makes the send transactions test fast enough, we can open a ticket to run it on every PR again.

docker/entrypoint.sh Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #5589 (50bb037) into main (13cd8b9) will decrease coverage by 0.17%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5589      +/-   ##
==========================================
- Coverage   78.80%   78.62%   -0.18%     
==========================================
  Files         305      305              
  Lines       38120    38178      +58     
==========================================
- Hits        30040    30017      -23     
- Misses       8080     8161      +81     

Co-authored-by: teor <teor@riseup.net>
@github-actions github-actions bot added the C-feature Category: New features label Nov 9, 2022
@arya2
Copy link
Contributor Author

arya2 commented Nov 9, 2022

If this PR makes the send transactions test fast enough, we can open a ticket to run it on every PR again.

I think ~30m is fast enough, I created issue #5600 for running it on every PR update.

@arya2
Copy link
Contributor Author

arya2 commented Nov 9, 2022

mempool_requests_for_transactions test failed in CI Docker / Test All:

failures:

---- components::inbound::tests::fake_peer_set::mempool_requests_for_transactions stdout ----

The application panicked (crashed).
Message: response to MempoolTransactionIds request should match added_transaction_ids [Legacy(transaction::Hash("c4eaa58879081de3c24a7b117ed2b28300e7ec4c4c1dff1d3f1268b7857a4ddb"))]
Location: zebrad/src/components/inbound/tests/fake_peer_set.rs:81

https://github.com/ZcashFoundation/zebra/actions/runs/3431178507/jobs/5719616560#step:3:9361

Issue #5384

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.

Looks good, let's merge!

mergify bot added a commit that referenced this pull request Nov 10, 2022
mergify bot added a commit that referenced this pull request Nov 10, 2022
mergify bot added a commit that referenced this pull request Nov 10, 2022
@mergify mergify bot merged commit c447b03 into main Nov 10, 2022
@mergify mergify bot deleted the update-ci-and-long-running-tests branch November 10, 2022 03:40
@teor2345
Copy link
Contributor

I added the new test to the branch protection rules for main.

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-bug Category: This is a bug C-enhancement Category: This is an improvement C-feature Category: New features C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add submitblock test to CI Get transactions from the non-finalized state in the send transactions test
2 participants