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

ink::test attributes in new template #190

Merged
merged 4 commits into from
Mar 30, 2021
Merged

Conversation

trace-andreason
Copy link
Contributor

Just adding the ink prefix to the test attributes in the new template. Pretty minor, but if you don't notice and you copy the #[test] attribute for a more complex test, your test will break in a confusing way.

@cmichi
Copy link
Collaborator

cmichi commented Feb 22, 2021

The problem here ist that the implication of #[ink::test] is that the test then is run in a simulated chain environment (we call this "off-chain testing"). Hence it comes with a bit of overhead for spawning this simulation. It's also not needed for every test, only for the ones where e.g. chain storage or the env() API is involved, that's why it sometimes breaks for you.

The tests for which you added the attribute in this PR don't require a simulated chain environment, as they only unit test isolated contract logic.

I acknowledge that it can be misleading to have the similarly named #[ink::test] and #[test] attributes. If we were to change this there would be two options:

  • Just replace #[test] with #[ink::test] for every single test and live with the slight overhead.
  • Improve naming with something like #[ink::simulated_chain] or so.

(cc @Robbepop, @ascjones)

@Robbepop
Copy link
Contributor

Improve naming with something like #[ink::simulated_chain] or so.

Hm, maybe if the current design is confusing we should consider renaming it or even change semantics.
E.g. the attribute will no longer put a #[test] on the function but instead query the marked function to have this attribute, bailing out if not so it is only applicable to unit tests.
Also renaming it to something like #[ink(emulate_chain)] could make sense but I am not sure about this.
How to find out what is less confusing? 🤔

@trace-andreason
Copy link
Contributor Author

@cmichi yeah I guess understand not adding it in the name of efficiency. What I would say though is that it's not totally clear to me what tests need to be ran in the simulated environment, and which do not. If things stay the same, it's probably safe to say that everyone who writes a smart contract is going to have a test blow up in their face and have to figure out why, which could be fine 🤷

Maybe a better solution would be to improve the error message you get when the test does need to be ran in the simulated environment:

---- erc20::tests::balance_works stdout ----
thread 'erc20::tests::balance_works' panicked at 'uninitialized execution context: UninitializedExecutionContext', /home/trace/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/ink_env-3.0.0-rc2/src/engine/off_chain/impls.rs:240:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@Robbepop
Copy link
Contributor

The simplest solution so far is: Write #[test]. Compile it. If it explodes use #[ink(test)]. Profit.

@trace-andreason
Copy link
Contributor Author

@Robbepop I have no problem with this issue being closed if the consensus is that this is just part of the learning curve

@Robbepop
Copy link
Contributor

We are aware that this is hard to learn and are very happy about your feedback. Ideally we use syntax that does not need to be taught or which can be taught in an autonomous or automated fashion, e.g. by some intelligent compiler errors or warnings.

@cmichi
Copy link
Collaborator

cmichi commented Feb 22, 2021

I like these two ideas:

(1) Rename #[ink::test]. Maybe something like this for better clarity:

#[test]
#[ink(emulate_chain)]
fn new_must_work() { … }

or

#[ink::emulated_chain_test]

(2) Improve the compiler error on uninitialized context to give hints about using the ink! test attribute.

Co-authored-by: Michael Müller <mich@elmueller.net>
Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this discussion up, @trace-andreason !

We're currently migrating all public facing contract tests to use #[ink::test].

@cmichi cmichi merged commit fa99483 into use-ink:master Mar 30, 2021
@cmichi cmichi mentioned this pull request Mar 31, 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.

3 participants