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

Support custom environment in E2E tests #1645

Merged
merged 19 commits into from
Feb 14, 2023

Conversation

pmikolajczyk41
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 commented Feb 8, 2023

This PR allows specifying a custom environment for an e2e test with:

#[ink_e2e::test(environment = crate::CustomEnvironment)]
async fn testcase(mut client: Client<C, E>) -> E2EResult<()> {
  ...
}

For an example, see examples/custom-environment.

Message building

Currently, ink_e2e exposes two APIs for message building:

  • MessageBuilder that is already generic over environment
  • build_message that prepares MessageBuilder for DefaultEnvironment

Generally, build_message is nothing more than MessageBuilder::<DefaultEnvironment, Ref>::from_account_id(account_id). Therefore, I wasn't sure whether we want do anything about it in the context of this PR - I'm open to any suggestion :)

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.

README of the example contracts needs an update; and just few neatpicks from me.
Otherwise, looks good :)

examples/custom-environment/README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
crates/e2e/macro/src/config.rs Outdated Show resolved Hide resolved
crates/e2e/macro/src/config.rs Outdated Show resolved Hide resolved
@pmikolajczyk41 pmikolajczyk41 requested review from SkymanOne and removed request for ascjones, cmichi and HCastano February 8, 2023 13:33
@HCastano HCastano changed the title [ink_e2e]: support custom environment Support custom environment in E2E tests Feb 8, 2023
@HCastano HCastano added the A-ink_e2e [ink_e2e] Work item label Feb 8, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #1645 (e289ded) into master (8bc8593) will decrease coverage by 0.03%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master    #1645      +/-   ##
==========================================
- Coverage   70.72%   70.69%   -0.03%     
==========================================
  Files         206      206              
  Lines        6404     6416      +12     
==========================================
+ Hits         4529     4536       +7     
- Misses       1875     1880       +5     
Impacted Files Coverage Δ
crates/e2e/macro/src/codegen.rs 0.00% <0.00%> (ø)
crates/e2e/macro/src/config.rs 84.84% <81.81%> (+12.84%) ⬆️
crates/allocator/src/bump.rs 85.95% <0.00%> (-3.13%) ⬇️
crates/ink/ir/src/ast/attr_args.rs 82.60% <0.00%> (-2.18%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Only took a quick look, will review again tomorrow

crates/e2e/macro/src/codegen.rs Outdated Show resolved Hide resolved
crates/e2e/macro/src/config.rs Outdated Show resolved Hide resolved
examples/custom-environment/README.md Outdated Show resolved Hide resolved
examples/custom-environment/lib.rs Show resolved Hide resolved
examples/custom-environment/Cargo.toml Outdated Show resolved Hide resolved
examples/custom-environment/Cargo.toml Show resolved Hide resolved
@ganesh1997oli
Copy link
Contributor

Are block_time in this llink which is 6 and timestamp while defining custom environment type Timestamp = <ink::env::DefaultEnvironment as Environment>::Timestamp; same? If so can it be possible to override that value?

@pmikolajczyk41
Copy link
Member Author

@ganesh1233456 these are very different things; block_time = 6 tells that every 6 seconds a new block should be created/finalized; Timestamp is a type used by e.g. pallet timestamp for adding real time info to the block

@pmikolajczyk41 pmikolajczyk41 requested review from HCastano and removed request for SkymanOne and HCastano February 9, 2023 08:49
examples/custom-environment/lib.rs Outdated Show resolved Hide resolved
examples/custom-environment/lib.rs Outdated Show resolved Hide resolved
examples/custom-environment/lib.rs Outdated Show resolved Hide resolved
examples/custom-environment/lib.rs Outdated Show resolved Hide resolved
examples/custom-environment/lib.rs Outdated Show resolved Hide resolved
crates/e2e/macro/src/config.rs Show resolved Hide resolved
}

#[test]
fn duplicate_environment_fails() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would also be cool to see some UI tests for the E2E macros, but that would be for a future PR

examples/custom-environment/lib.rs Outdated Show resolved Hide resolved
examples/custom-environment/lib.rs Outdated Show resolved Hide resolved
crates/e2e/macro/src/config.rs Outdated Show resolved Hide resolved
@pmikolajczyk41 pmikolajczyk41 requested review from HCastano and removed request for SkymanOne February 9, 2023 22:48
@pmikolajczyk41 pmikolajczyk41 requested review from SkymanOne and removed request for HCastano February 10, 2023 13:20
Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Almost there!

crates/e2e/macro/src/config.rs Outdated Show resolved Hide resolved
crates/e2e/macro/src/config.rs Show resolved Hide resolved
examples/custom-environment/lib.rs Outdated Show resolved Hide resolved
}
}

#[cfg(all(test, feature = "e2e-tests", feature = "permissive-node"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also be curious to know what happens if we try and run an E2E test with a custom env on a node that doesn't use one

Copy link
Member Author

Choose a reason for hiding this comment

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

test added

Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@HCastano
Copy link
Contributor

HCastano commented Feb 13, 2023

@SkymanOne since you requested changes we still need another review from you before merging

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.

LGTM!

@HCastano HCastano merged commit 21606c0 into use-ink:master Feb 14, 2023
@pmikolajczyk41 pmikolajczyk41 deleted the pmikolajczyk41/e2e/custom-env branch February 14, 2023 06:18
@ascjones ascjones mentioned this pull request Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ink_e2e [ink_e2e] Work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants