Use BankForks on tests - Part 2 #34234
Conversation
Signed-off-by: Lucas Steuernagel <lucas.tnagel@gmail.com>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #34234 +/- ##
========================================
Coverage 81.9% 81.9%
========================================
Files 819 819
Lines 219350 219426 +76
========================================
+ Hits 179698 179802 +104
+ Misses 39652 39624 -28 |
runtime/src/bank.rs
Outdated
| @@ -977,6 +977,21 @@ impl Bank { | |||
| (bank_arc, bank_fork) | |||
| } | |||
|
|
|||
| #[cfg(feature = "dev-context-only-utils")] | |||
| pub fn new_from_parent_with_bank_forks( | |||
There was a problem hiding this comment.
Let's not add this to Bank as a constructor. This function is using public methods from Bank and BankForks. So it can be easily implemented as part of the tests.
The test code can carry this function as a utility function. We can copy the function to different test files (in case crate boundary is causing it to be inaccessible).
There was a problem hiding this comment.
You nailed my problem. I did not want to repeat code between crates, so I guess this is the solution after all.
There was a problem hiding this comment.
Generally we should avoid repeating the code. In this particular case, adding the constructor to Bank seems incorrect, since it's a test-only code. And, it's two lines of code that calls the public functions. So it seems ok to copy the function to tests.
There was a problem hiding this comment.
I repeated the function. Let me know what you think.
Problem
In order for us to implement #34169 (remove
WorkSlotfromLoadedPrograms::extract), we need to make sure all tests create aBankForkand use aBankinstance that has been added to the fork.Summary of Changes
This PR adds a few auxiliary functions to deal with
BankForksand fixes some tests. This is the continuation of the work I started on #34206 .