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

Replaces MockInvokeContext by ThisInvokeContext in tests #20881

Merged
merged 9 commits into from
Nov 4, 2021

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Oct 22, 2021

Problem

MockInvokeContext would need its own ABIv2 compatibility and still uses KeyedAccounts.
So going forward it is easier to just route all the tests to use ThisInvokeContext instead, so that ABIv2 and the inter-operation with the old ABI can both be tested in one structure / implementation.

Summary of Changes

  • Replaces use sites of MockInvokeContext by ThisInvokeContext.
  • Removes MockInvokeContext, MockComputeMeter and MockLogger.
  • Moves ThisInvokeContext to the program-runtime crate to avoid a cyclic dependency with the runtime crate in the program tests.

Fixes #

@Lichtso Lichtso changed the title Remove MockInvokeContext Replaces MockInvokeContext by ThisInvokeContext in tests Oct 22, 2021
@Lichtso Lichtso force-pushed the refactor/MockInvokeContext branch 9 times, most recently from e564ef5 to 6ae28d9 Compare October 27, 2021 22:14
@Lichtso Lichtso requested a review from jackcmay October 27, 2021 22:28
@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

Merging #20881 (9c29e44) into master (29ad081) will increase coverage by 0.0%.
The diff coverage is 97.4%.

@@           Coverage Diff           @@
##           master   #20881   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         497      498    +1     
  Lines      140320   140238   -82     
=======================================
- Hits       114440   114379   -61     
+ Misses      25880    25859   -21     

@Lichtso Lichtso force-pushed the refactor/MockInvokeContext branch 13 times, most recently from 89d4aa2 to 3d92394 Compare October 30, 2021 09:08
@Lichtso Lichtso force-pushed the refactor/MockInvokeContext branch 3 times, most recently from 30f15bf to cf3ea97 Compare November 3, 2021 18:14
@@ -1,8 +1,8 @@
#![cfg_attr(RUSTC_WITH_SPECIALIZATION, feature(min_specialization))]
#![allow(clippy::integer_arithmetic)]
#![allow(clippy::integer_arithmetic)] // TODO: Remove
Copy link
Contributor

Choose a reason for hiding this comment

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

Still needs to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have another branch for a follow-up PR on this one, which is going to move everything from sdk::process_instruction into the program-runtime-crate. And it will remove that clippy flag.

https://github.com/Lichtso/solana/tree/refactor/move_process_instruction_in_program_runtime

);

// Case: Account not a program
keyed_accounts[0].3.borrow_mut().set_executable(false);
assert_eq!(
Err(InstructionError::InvalidInstructionData),
process_instruction(&loader_id, &[], &keyed_accounts),
Err(InstructionError::IncorrectProgramId),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice: The error code changed as it was previously testing:
process_instruction(&loader_id, &[], &[], &keyed_accounts)
which is incorrect and was changed to:
process_instruction(&loader_id, &[0], &[], &keyed_accounts)

This happened because it originally tested with &loader_id instead of &program_key which the other tests used.

Copy link
Contributor

@jackcmay jackcmay left a comment

Choose a reason for hiding this comment

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

I think this looks good, I don't see anything blaring but it's very hard to review when the PR is so large and includes so many unrelated changes.

It would have been helpful if the following had been broken out into separate PRs so that it is easier to identify functional changes:

  • Wholesale changes from new_rand() to new_unique()
  • Switching to new_ref()
  • Renames and code changes unrelated to the problem description
  • Moving files around and making large changes

@Lichtso
Copy link
Contributor Author

Lichtso commented Nov 4, 2021

had been broken out into separate PRs

Already trying, in fact this is already broken up from a much larger refactoring I have locally.
The other parts are:

Wholesale changes from new_rand() to new_unique()

I agree, that should have been its own PR. I simply forgot that I changed it to have deterministic tests in order to compare that before and after are equal.

Switching to new_ref()

ThisInvokeContext::push() requires Rc<RefCell<AccountSharedData>> which MockInvokeContext didn't so it has to happen in this PR.

Moving files around and making large changes

If you mean the moving of ThisInvokeContext into the program-runtime-crate:
I tried that, but while rebasing this PR onto the immediate state of just moving ThisInvokeContext and separating that from the changes to dyn Trait InvokeContext I decided against it because I still had dependency cycles at the time. But yes, it could have been separated after wards.


I do certainly realize how much happened in this PR and that it was hard to review. I am very grateful!
I think the coming follow-up PRs will be a lot cleaner because I have a better idea what they entail.

@Lichtso Lichtso merged commit 7200c51 into solana-labs:master Nov 4, 2021
@Lichtso Lichtso deleted the refactor/MockInvokeContext branch November 4, 2021 20:47
This was referenced Nov 9, 2021
dankelleher pushed a commit to identity-com/solana that referenced this pull request Nov 24, 2021
…#20881)

* Replaces MockInvokeContext by ThisInvokeContext in BpfLoader, SystemInstructionProcessor, CLIs, ConfigProcessor, StakeProcessor and VoteProcessor.

* Finally, removes MockInvokeContext, MockComputeMeter and MockLogger.

* Adjusts assert_instruction_count test.

* Moves ThisInvokeContext to the program-runtime crate.
frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants