-
Notifications
You must be signed in to change notification settings - Fork 137
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
Enrich test framework interface #2505
Enrich test framework interface #2505
Conversation
I would like the team's input, on how to advance with this blocker: https://github.com/onflow/cadence/pull/2505/files#diff-48d2de22767ded1c341778e03aee3e1d3e1006ceeae566f4b0a3a7704c60758dR642-R646. Is there any workaround? |
Codecov Report
@@ Coverage Diff @@
## master #2505 +/- ##
==========================================
- Coverage 78.36% 78.33% -0.03%
==========================================
Files 336 327 -9
Lines 77123 73393 -3730
==========================================
- Hits 60436 57492 -2944
+ Misses 14429 13795 -634
+ Partials 2258 2106 -152
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some tests for the new functions where possible (e.g. serviceAccount()
)?
Please ignore, just realized we need the blockchain for that. So the test has to be on the tools repo 👍
6b47d46
to
7c9d29e
Compare
This function will call the emulator's Blockchain.ReloadBlockchain method, under the hood.
7c9d29e
to
acc49f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Would it be possible to add some test cases to cover the added code?
@turbolent I am not sure how would that be possible here, since we do not have a pub let events = blockchain.events()
pub let specificEvents = blockchain.eventsOfType(type)
pub let serviceAccount = blockchain.serviceAccount()
blockchain.logs()
blockchain.reset() That's why most of the tests are located in the |
For example: https://github.com/onflow/cadence-tools/blob/master/test/test_framework_test.go#L684-L700, test case for the |
@m-Peter I see. Maybe we can add a mock/test implementation in a follow-up PR? It would be good to not have to rely on some downstream dependencies being the own tests (i.e. performing integration tests), and also have some unit tests up here. Doesn't block this PR, just some general thoughts |
@turbolent You are completely right, I think it will surely increase the developer's confidence for the added code. I will follow up with PR to tackle this 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! left a suggestion for an improvement, which can be a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Work towards: onflow/developer-grants#148
Companion PRs:
Blockchain
events cadence-tools#113Blockchain
logs cadence-tools#114Description
Adds 5 new methods to the
TestFramework
interface, and their native implementations.master
branchFiles changed
in the Github PR explorer