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

Use BankForks on tests - Part 1 #34206

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Use BankForks on tests - Part 1 #34206

merged 2 commits into from
Nov 27, 2023

Conversation

LucasSte
Copy link
Contributor

Problem

In order for us to implement #34169 (remove WorkSlot from LoadedPrograms::extract), we need to make sure all tests create a BankFork and use a Bank instance that has been added to the fork.

Summary of Changes

This PR adds a few auxiliary functions to deal with BankForks and fixes some tests. Another PR that fixes more tests is waiting for this one to be merged.

Signed-off-by: Lucas Steuernagel <[email protected]>
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Merging #34206 (88b4cb9) into master (53c723a) will decrease coverage by 0.1%.
Report is 6 commits behind head on master.
The diff coverage is 98.2%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34206     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         819      819             
  Lines      219425   219471     +46     
=========================================
+ Hits       179854   179869     +15     
- Misses      39571    39602     +31     

@LucasSte LucasSte marked this pull request as ready for review November 23, 2023 13:42
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
@@ -694,6 +695,16 @@ impl BankForks {
&& bank.parent_slot() < start_slot
&& bank.slot() >= start_slot
}

pub fn new_bank_from_parent_for_tests(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since new_with_fork_for_tests() returns the instance of BankForks, we can insert the new bank to forks in the tests itself. It's just two lines of code at the various call sites.

Copy link
Contributor Author

@LucasSte LucasSte Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know two lines does not sound a lot, but function this called 57 times after fixing all the tests in the runtime folder. It facilitates writing tests, making them conciser.

If not as a method of BankForks I can code it as standalone function or add the #[cfg(tests)] attribute. Is there any chance I can keep the function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about move it to tests.rs as a utility function? Keeping it it bank_forks increases the possibility of accidental misuse.

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't read through the majority of the code, but...

For any new test-only functions that cannot go into a tests submodule, can they be annotated with dev-context-only-utils?

Here's an example:

solana/runtime/src/bank.rs

Lines 4250 to 4253 in 57ec207

#[cfg(feature = "dev-context-only-utils")]
pub fn register_tick_for_test(&self, hash: &Hash) {
self.register_tick(hash, &BankWithScheduler::no_scheduler_available())
}

Code that is annotated with DCOU, is only usable where the DCOU feature (in the rust sense) is enabled, which prevents non-tests and non-dev-tools from accidentally calling these functions.

@pgarg66
Copy link
Contributor

pgarg66 commented Nov 27, 2023

I didn't read through the majority of the code, but...

For any new test-only functions that cannot go into a tests submodule, can they be annotated with dev-context-only-utils?

Here's an example:

solana/runtime/src/bank.rs

Lines 4250 to 4253 in 57ec207

#[cfg(feature = "dev-context-only-utils")]
pub fn register_tick_for_test(&self, hash: &Hash) {
self.register_tick(hash, &BankWithScheduler::no_scheduler_available())
}

Code that is annotated with DCOU, is only usable where the DCOU feature (in the rust sense) is enabled, which prevents non-tests and non-dev-tools from accidentally calling these functions.

That's a great point. It's specially useful if someone wants to export these utility function to other crates. I suspect there are tests in other crates that will need these test_only constructors. When modifying those tests, DCOU can be used to make these functions accessible.

@LucasSte LucasSte merged commit e832765 into solana-labs:master Nov 27, 2023
32 checks passed
@LucasSte LucasSte deleted the fork-part-1 branch November 27, 2023 17:10
@pgarg66 pgarg66 added the v1.17 PRs that should be backported to v1.17 label Dec 7, 2023
Copy link
Contributor

mergify bot commented Dec 7, 2023

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify bot pushed a commit that referenced this pull request Dec 7, 2023
---------

Signed-off-by: Lucas Steuernagel <[email protected]>
(cherry picked from commit e832765)

# Conflicts:
#	runtime/src/bank/tests.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants