-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Moves Bank benches-only ctors in DCOU #34545
Conversation
71b287d
to
b780424
Compare
b780424
to
d85b441
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #34545 +/- ##
=========================================
- Coverage 81.8% 81.8% -0.1%
=========================================
Files 822 822
Lines 221540 221540
=========================================
- Hits 181403 181362 -41
- Misses 40137 40178 +41 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mentioned another that i think should be moved in comment. also new_with_paths_for_tests
?
pub fn new_for_benches(genesis_config: &GenesisConfig) -> Self { | ||
Self::new_with_paths_for_benches(genesis_config, Vec::new()) | ||
} | ||
|
||
/// Intended for use by tests only. | ||
/// create new bank with the given configs. | ||
pub fn new_with_runtime_config_for_tests( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it doesn't fit the title, but shouldn't new_with_runtime_config_for_tests
also be either test or dcou?
Oh absolutely yes. Unfortunately it's not that straightforward... Here's what needs to merge first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - thanks for the justification on not moving the other fns yet
Problem
Bank has constructors that are only called by benches. These should be in a DCOU block.
Summary of Changes
Put 'em in a DCOU block.