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

evm: Add tests related to contracts size #2380

Merged
merged 26 commits into from
Sep 6, 2023
Merged

Conversation

cuongquangnam
Copy link
Contributor

@cuongquangnam cuongquangnam commented Aug 24, 2023

Summary

  • Test deployment of various contract sizes

Implications

  • Storage

    • Database reindex required
    • Database reindex optional
    • Database reindex not required
    • None
  • Consensus

    • Network upgrade required
    • Includes backward compatible changes
    • Includes consensus workarounds
    • Includes consensus refactors
    • None

@cuongquangnam cuongquangnam marked this pull request as ready for review August 25, 2023 06:04
@canonbrother
Copy link
Contributor

GREAT for this changes!! 1463572

@canonbrother
Copy link
Contributor

can we add a time measurement in each test?? (more info req)

test/functional/feature_evm_contracts.py Outdated Show resolved Hide resolved
test/functional/feature_evm_contracts.py Outdated Show resolved Hide resolved
test/functional/feature_evm_contracts.py Outdated Show resolved Hide resolved
test/functional/feature_evm_contracts.py Outdated Show resolved Hide resolved
@cuongquangnam
Copy link
Contributor Author

cuongquangnam commented Aug 28, 2023

can we add a time measurement in each test?? (more info req)

Why do we need time measurements? @canonbrother

@canonbrother
Copy link
Contributor

can we add a time measurement in each test?? (more info req)

Why do we need time measurements? @canonbrother

@cuongquangnam
for benchmarking purpose.. just a reference for us to know deployment time against the bytecode size

easier as

start_time = time.time()
// code snippet...
end_time = time.time()
elapsed_time = end_time - start_time
print("Elapsed time: {} s".format(elapsed_time))

@shohamc1
Copy link
Contributor

Don't think we need this as part of the test? We can add it ad-hoc when needed.

canonbrother
canonbrother previously approved these changes Aug 29, 2023
@prasannavl prasannavl added the v/next-release Items ready or targeted for upcoming release(s) label Aug 29, 2023
test/functional/feature_evm_contracts.py Outdated Show resolved Hide resolved
test/functional/feature_evm_contracts.py Outdated Show resolved Hide resolved
@prasannavl
Copy link
Member

prasannavl commented Aug 30, 2023

f4aff43

On this, I'd actually prefer the localized var node than repeated self. It's a good practise to pull all vars in the beginning of the function into the localized context and use those.

However @shohamc1 how about extrapolating on your idea and setting self.node directly into the test framework that aliases nodes[0] or is None?

@shohamc1
Copy link
Contributor

shohamc1 commented Sep 4, 2023

f4aff43

On this, I'd actually prefer the localized var node than repeated self. It's a good practise to pull all vars in the beginning of the function into the localized context and use those.

However @shohamc1 how about extrapolating on your idea and setting self.node directly into the test framework that aliases nodes[0] or is None?

Maybe we can do this on a different PR instead? This one is quite large as it is right now.

@prasannavl
Copy link
Member

Sure, sounds good to me

shohamc1
shohamc1 previously approved these changes Sep 5, 2023
@prasannavl prasannavl merged commit 3c2459a into master Sep 6, 2023
11 of 13 checks passed
@prasannavl prasannavl deleted the cuong/add_tests_contracts branch September 6, 2023 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v/next-release Items ready or targeted for upcoming release(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants