test(sns-cli): Port UpgradeSnsControlledCanister with Large Wasm integration test to use SNS CLI with PocketIc #3696
Conversation
aterga
force-pushed
the
arshavir/test-new-cli-3
branch
from
b9e6575 to
5c97ccf
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).
To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:
-
Done.
-
No canister behavior changes.
aterga
dismissed
github-actions[bot]’s stale review
No canister behavior changes.
| let controllers = self.pocket_ic.get_controllers(canister_id).await; | ||
|
|
||
| let Some(controller) = controllers.into_iter().last() else { | ||
| return Err(Self::Error::BlackHole); |
There was a problem hiding this comment.
doesn't a blackholed canister still have a visible module_hash? Should this be treated as an error?
There should be a way, in the same way that we can query the IC mainnet canisters, to get that information without being a controller
There was a problem hiding this comment.
The name "blackhole" could be a bit misleading as a blackhole canister typically still has a controller, e.g., itself, to fetch the module hash. This case is about a canister with no controller at all: in this case, we cannot use the mgmt canister to retrieve the module hash (only controllers can call the mgmt canister) and thus we can only make a read state request which is possible, but substantially more complex (see an example of getting subnet metrics which can only be retrieved by read state requests and thus the complexity is unavoidable). Yet another alternative would be to retrieve the module hash from the raw state directly (analogously to controllers).
| @@ -30,7 +30,12 @@ async fn main() -> Result<()> { | |||
| SubCommand::List(args) => list::exec(args, &agent).await, | |||
| SubCommand::Health(args) => health::exec(args, &agent).await, | |||
| SubCommand::UpgradeSnsControlledCanister(args) => { | |||
| upgrade_sns_controlled_canister::exec(args, &agent).await | |||
| match upgrade_sns_controlled_canister::exec(args, &agent).await { | |||
There was a problem hiding this comment.
I think with a little restructuring, you could have main only pass 'std::env::args()' into a function that parses and runs the function.
Then you could test, in unit tests, everything else (assuming you have a little bit of dependency injection for things with side effects you don't want to run in tests).
NCR, obviously, just an observation for future PRs
This PR leverages the SNS CLI functionality to prepare and submit UpgradeSnsControlledCanister proposals for large Wasms as part of the existing integration test scenario. The goal is to have the CLI functionality tested in CI. This entails the need to generalize the CLI command upgrade-sns-controlled-canister to work with a CallCanister instance which can be instantiated not only with ic-agent, but also with PocketIc instances.