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

NV18: FEVM: Basic smoke test #9635

Merged
merged 7 commits into from
Nov 14, 2022
Merged

NV18: FEVM: Basic smoke test #9635

merged 7 commits into from
Nov 14, 2022

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Nov 14, 2022

The basic FEVM smoke test with SimpleCoin.

@vyzo vyzo requested review from Stebalien and raulk November 14, 2022 10:37
@vyzo vyzo requested a review from a team as a code owner November 14, 2022 10:37
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Oh no, this test is far too CLI-y and string-matchy for my preference. This is problematic for 2 reasons:

  • Such tests are notoriously brittle and lead to future 😿
  • Such tests are less useful as "demonstrators" of how things work (what messages are sent, what's returned, etc.)

I think it would be much nicer to actually read the contract bytes, formulate the messages as appropriate, and parse returns with assertions. This will definitely be a more bloated test (much more code), but overall better.

(If we want this as a separate CLI test, just to cover CLI functionality, I'm less opposed to that).

@vyzo
Copy link
Contributor Author

vyzo commented Nov 14, 2022

Basically you want me to copy the cli code ;)

I guess I could do that.

@vyzo
Copy link
Contributor Author

vyzo commented Nov 14, 2022

But yes, we do want to cover cli functionality too.
We have been using this as a smoke test for a long time.

@Stebalien
Copy link
Member

But yes, we do want to cover cli functionality too.

That would go in cli/chain_test.go. We usually test the API and CLI separately.

@vyzo
Copy link
Contributor Author

vyzo commented Nov 14, 2022

It is an itest however, and there plenty of cli tests in the itest bundle.

I propose we keep this here and add a second test that does everything by hand.

@arajasek
Copy link
Contributor

It is an itest however, and there plenty of cli tests in the itest bundle.

I propose we keep this here and add a second test that does everything by hand.

Yeah, I don't think there should be, and don't really want more. Here's a good test to use as a model -- it's basically laid out as:

  • setup
  • push message, assertion
  • push message, assertion
  • migration (not relevant to this PR), assertion
  • push moar messages, moar assertions.

@vyzo
Copy link
Contributor Author

vyzo commented Nov 14, 2022

ok, I rewrote the thing as requested and removed the cli test; I will squash on merge and there will be no sign of it....

itests/fevm_test.go Outdated Show resolved Hide resolved
itests/fevm_test.go Outdated Show resolved Hide resolved
itests/fevm_test.go Outdated Show resolved Hide resolved
@vyzo vyzo merged commit af39ec2 into feat/nv18-fevm Nov 14, 2022
@vyzo vyzo deleted the nv18-fevm-smoke-test branch November 14, 2022 19:06
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.

5 participants