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

Add E2E testing framework MVP #1395

Merged
merged 20 commits into from
Sep 28, 2022
Merged

Add E2E testing framework MVP #1395

merged 20 commits into from
Sep 28, 2022

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Sep 14, 2022

MVP for #1234.

I've created a fresh PR to make it easier for reviews.

Contains only E2E tests for our contract-transfer example so far, no cross-contract tests yet. Once we've nailed down the API I'll add E2E tests for the other examples in follow-up PRs. The best way to look at this PR is, I think, to look at the #[ink::e2e_test] tests for the contract-transfer example.

I've annotated a couple of TODO(#xxx) comments ‒ I'll create the issues + add the number before merging, I'm just waiting for approvals beforehand.

crates/env/src/engine/e2e/xts.rs Outdated Show resolved Hide resolved
crates/env/src/engine/e2e/xts.rs Show resolved Hide resolved
crates/env/Cargo.toml Show resolved Hide resolved
@cmichi
Copy link
Collaborator Author

cmichi commented Sep 19, 2022

CI currently fails because use-ink/cargo-contract#728 is not yet merged.

Copy link
Contributor

@athei athei left a comment

Choose a reason for hiding this comment

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

You can't really collect the output of a contract call with this, right? Because you only dry-run for gas_limit but do not allow to explicitly dry-run for calling non mutable massages.

crates/env/src/engine/e2e/xts.rs Show resolved Hide resolved
crates/env/src/engine/e2e/xts.rs Show resolved Hide resolved
Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

These are my thoughts. I understand that we don't want to loosely couple cargo-contract and ink, but I personally see some code duplication as an issue because we'll have to change the code in 2 different places if specs are changed.

crates/env/Cargo.toml Outdated Show resolved Hide resolved
crates/env/src/engine/e2e/xts.rs Show resolved Hide resolved
crates/env/src/engine/e2e/xts.rs Show resolved Hide resolved
crates/env/src/engine/e2e/xts.rs Show resolved Hide resolved
crates/env/src/engine/e2e/xts.rs Show resolved Hide resolved
@cmichi
Copy link
Collaborator Author

cmichi commented Sep 26, 2022

You can't really collect the output of a contract call with this, right? Because you only dry-run for gas_limit but do not allow to explicitly dry-run for calling non mutable massages.

@athei Can't fully follow you here, can you elaborate?

@athei
Copy link
Contributor

athei commented Sep 26, 2022

You can't really collect the output of a contract call with this, right? Because you only dry-run for gas_limit but do not allow to explicitly dry-run for calling non mutable massages.

@athei Can't fully follow you here, can you elaborate?

It looks to me that you are not returning the result (as in the output buffer) to the user of the testing framework.

@cmichi
Copy link
Collaborator Author

cmichi commented Sep 27, 2022

You can't really collect the output of a contract call with this, right? Because you only dry-run for gas_limit but do not allow to explicitly dry-run for calling non mutable massages.

@athei Can't fully follow you here, can you elaborate?

It looks to me that you are not returning the result (as in the output buffer) to the user of the testing framework.

The return value of invoking call() on the E2E testing framework is a Result with an Ok of:

/// Result of a contract call.
pub struct CallResult<C: subxt::Config, E: Environment> {
    /// The result of the dry run, contains debug messages
    /// if there were any.
    pub dry_run: ContractExecResult<E::Balance>,
    /// Events that happened with the contract instantiation.
    pub events: TxEvents<C>,
}

The dry_run here is all that the state_call RPC for ContractsApi_call returns. This is how CallResult.dry_run looks:

ContractResult {
        gas_consumed: 5501174466,
        gas_required: 74999922688,
        storage_deposit: StorageDeposit::Charge(0),
        debug_message: [],
        result: Ok(
                ExecReturnValue {
                        flags: (empty),
                        data: Bytes([0, 57, 5, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
                }
        )
}

So all of this is returned to the user as part of executing .call(). This should cover anything, right?

@athei
Copy link
Contributor

athei commented Sep 27, 2022

So all of this is returned to the user as part of executing .call(). This should cover anything, right?

yes

@cmichi cmichi merged commit 64fbe40 into master Sep 28, 2022
@cmichi cmichi deleted the cmichi-mvp-e2e-testing branch September 28, 2022 02:12
Comment on lines +53 to +72
contract-metadata = "2.0.0-alpha.2"
impl-serde = { version = "0.3.1", default-features = false }
jsonrpsee = { version = "0.14.0", features = ["ws-client"] }
pallet-contracts-primitives = "6.0.0"
serde = { version = "1.0.137", default-features = false, features = ["derive"] }
serde_json = "1.0.81"
tokio = { version = "1.18.2", features = ["rt-multi-thread"] }
log = "0.4"
env_logger = "0.8"
subxt = "0.24.0"

# Substrate
sp-rpc = "6.0.0"
sp-core = "6.0.0"
sp-keyring = "6.0.0"
sp-runtime = "6.0.0"

# TODO(#xxx) `smart-bench_macro` needs to be forked.
smart-bench-macro = { git = "https://github.com/paritytech/smart-bench", branch = "cmichi-ink-e2e-test-mvp", package = "smart-bench-macro"}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think all of these new e2e dependencies should be optional = true, since the e2e code is conditional for std.

Arguably we may even want an additional feature to prevent them being compiled for the metadata generation which is std, there are some hefty dependencies here.

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.

4 participants