-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
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.