Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Remove test_replay #2740

Closed

Conversation

sagar-solana
Copy link
Contributor

@sagar-solana sagar-solana commented Feb 12, 2019

Problem

test_replay() was not exactly testing anything.

It had two parts

  • Simulate a bank transfers manually (with ticks)
  • Test retransmits between dummy validator nodes

Bank has enough transfer tests and there was nothing being asserted for the retransmits making this test useless.

Summary of Changes

Removed the test. What ever this test was trying to do is covered elsewhere.

Fixes #2358

@codecov
Copy link

codecov bot commented Feb 12, 2019

Codecov Report

Merging #2740 into master will increase coverage by 0.4%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #2740     +/-   ##
========================================
+ Coverage    78.5%   78.9%   +0.4%     
========================================
  Files         115     115             
  Lines       19114   19003    -111     
========================================
- Hits        15013   15004      -9     
+ Misses       4101    3999    -102

@sakridge
Copy link
Contributor

Where else do we test that blobs get to the bank correctly in TVU?

@sagar-solana
Copy link
Contributor Author

sagar-solana commented Feb 12, 2019

@sakridge, this test wasn't even testing that(maybe it was designed to at one point?). It just makes the bank process the transfers directly. The balances will always be correct so those asserts were pointless. The tvu wasn't really doing anything here as far as I can tell.
Also the test freezes because storage_stage is stuck trying to get last_id from a bogus leader node.

I guess either way we'll need new replay stage tests with Bank forking coming in.

@sakridge
Copy link
Contributor

@sagar-solana I'm not sure what you mean by process transfers directly because it's not doing like bank.process_transaction, it's generating the blobs and sending them on the socket. The last_id thing could be fixed.

@sagar-solana
Copy link
Contributor Author

@sakridge, sorry I should have specified that this test is calling process_tick on the bank. Which is incorrect. That stops the tvu from being able to process the entries that are generated (since it's bank's last_id has moved before it even got the entries).

Maybe I can remove the bank calls and let tvu handle it all. But still we have to deal with the fact that storage_stage freezes after those two asserts when tvu.close() is called.

@sagar-solana
Copy link
Contributor Author

@garious, @sakridge pointed out that this test might actually have some value. Its just that with all the churn it's ended up becoming something else. I'll take another stab at fixing it.
Current outstanding problem with it is that the test can't shutdown properly because it's expecting the leader's rpc to be real.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants