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

[Flow EVM] Improving evm integration tests #5431

Merged
merged 18 commits into from
Mar 5, 2024

Conversation

ramtinms
Copy link
Contributor

@ramtinms ramtinms commented Feb 21, 2024

closes: #5228

It also adds some utility methods for integration testing that can be used later to expand the integration tests.

@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.85%. Comparing base (649345d) to head (2c10fd0).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5431      +/-   ##
==========================================
+ Coverage   55.30%   55.85%   +0.55%     
==========================================
  Files         946     1029      +83     
  Lines       96206   100783    +4577     
==========================================
+ Hits        53204    56292    +3088     
- Misses      38877    40159    +1282     
- Partials     4125     4332     +207     
Flag Coverage Δ
unittests 55.85% <ø> (+0.55%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramtinms ramtinms changed the title Ramtin/5228 improve evm integration tests [Flow EVM] improving evm integration tests Feb 22, 2024
@ramtinms ramtinms changed the title [Flow EVM] improving evm integration tests [Flow EVM] Improving evm integration tests Feb 22, 2024
@ramtinms ramtinms marked this pull request as ready for review February 28, 2024 19:51
code: [],
gasLimit: 53000,
value: EVM.Balance(attoflow: 1230000000000000000)
let bal = EVM.Balance(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let bal = EVM.Balance(0)
let bal = EVM.Balance(attoflow: 0)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a breaking change in Cadence 1.0. The named attribute is always required.

let cadenceOwnedAccount <- EVM.createCadenceOwnedAccount()
cadenceOwnedAccount.deposit(from: <-vault)

let bal = EVM.Balance(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let bal = EVM.Balance(0)
let bal = EVM.Balance(attoflow: 0)

) uint64 {
code := []byte(fmt.Sprintf(
`
pub fun main(): UFix64 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub fun main(): UFix64 {
access(all) fun main(): UFix64 {

Copy link
Collaborator

@m-Peter m-Peter left a comment

Choose a reason for hiding this comment

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

Very nice 👏
Left only a few comments, to make the Cadence changes compatible with C1.0.


assert(res.status == EVM.Status.failed, message: "unexpected status")
// ExecutionErrCodeExecutionReverted
assert(res.errorCode == 306, message: "unexpected error code")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe:

Suggested change
assert(res.errorCode == 306, message: "unexpected error code")
assert(res.errorCode == %d, message: "unexpected error code")
// ...
types.ExecutionErrCodeExecutionReverted


let bal = EVM.Balance(0)
bal.setFLOW(flow: 1.23)
let vault2 <- cadenceOwnedAccount.withdraw(balance: bal)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe you can add a line in Cadence that asserts the coa balance is 2.34-1.23.

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

very nice tests

@franklywatson franklywatson added this pull request to the merge queue Mar 5, 2024
Merged via the queue into master with commit 26a5cc0 Mar 5, 2024
51 checks passed
@franklywatson franklywatson deleted the ramtin/5228-improve-evm-integration-tests branch March 5, 2024 16:05
@ramtinms
Copy link
Contributor Author

ramtinms commented Mar 5, 2024

I'll apply suggested these in my other PR #5443

@ramtinms
Copy link
Contributor Author

ramtinms commented Mar 5, 2024

applied fixes to 7ae2c70

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.

[Flow EVM] Adding integration test cases
6 participants