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

[test] Add support for retrieving stdlib events #138

Merged
merged 6 commits into from
Jun 9, 2023

Conversation

m-Peter
Copy link
Contributor

@m-Peter m-Peter commented Jun 7, 2023

Description

  • Fix regex issue, when parsing non-string log messages
  • Add support for stdlib events that have type flow.*
  • Allow importing the Test contract in helper files
  • Update EmulatorBackend.Reset() function to use Blockchain.RollbackToBlockHeight() instead of Blockchain.ReloadBlockchain()
  • Improve CoverageReport metrics to include only contracts

Such events, can even be printed to the console:

1:04PM DBG LOG: flow.AccountCreated(address: 0x01cf0e2f2f715450)

Screenshot from 2023-06-07 13-04-49


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@m-Peter
Copy link
Contributor Author

m-Peter commented Jun 7, 2023

@SupunS I have added a simplified version to check the case which you described in #113 (comment). Even though we do create a new Test.Blockchain struct, with every call of Test.newEmulatorBlockchain(), the underlying backend implementation is always the same (per test-file that is, because on flow-cli I do create a new TestRunner for each test-file). What do you think? Are we covered here?

@SupunS
Copy link
Member

SupunS commented Jun 7, 2023

I have added a simplified version to check the case which you described in #113 (comment). Even though we do create a new Test.Blockchain struct, with every call of Test.newEmulatorBlockchain(), the underlying backend implementation is always the same (per test-file that is, because on flow-cli I do create a new TestRunner for each test-file). What do you think? Are we covered here?

Oh, that looks like a bug in the original implementation. Test.newEmulatorBlockchain() should ideally return a fresh backend instance everytime. We don't have to fix it here, but maybe can you report an issue for this for now?

@m-Peter
Copy link
Contributor Author

m-Peter commented Jun 7, 2023

Oh, that looks like a bug in the original implementation. Test.newEmulatorBlockchain() should ideally return a fresh backend instance everytime. We don't have to fix it here, but maybe can you report an issue for this for now?

In fact, the backend instance is created once in the ContractValueHandler, when the Test contract is first encountered in a test file. To me, it makes sense to provide just a Test.Blockchain singleton in every test file. Especially now that Reset() is supported. Instead of Test.newEmulatorBlockchain, just Test.emulatorBlockchain. That's a breaking change for the API though 😇 . What is your opinion on this? What's the best approach to solve this? I am asking so I can put it in an issue.

@SupunS
Copy link
Member

SupunS commented Jun 7, 2023

That is certainly an alternative solution. But then again, if the emulatorBlockchain is a singleton, we would not even need to expose it. i.e. instead of Test.emulatorBlockchain.executeScript() we can do Test.executeScript() because anyway only one blockchain is used.

We can open the issue just by mentioning the current problem (i.e: newEmulatorBlockchain returns the same instance) and then discuss the details/different solutions in the issue itself.
I can open the issue if you want me to :)

@m-Peter
Copy link
Contributor Author

m-Peter commented Jun 7, 2023

@SupunS #139 🙌

@m-Peter m-Peter force-pushed the add-support-for-stdlib-events branch from 80c39ef to 8647d13 Compare June 8, 2023 10:27
@m-Peter
Copy link
Contributor Author

m-Peter commented Jun 8, 2023

I have complemented the test suite with some tests for the Blockchain.reset method, and I had to change its implementation to get the desired functionality. I think this should be all for this PR 🙏
cc @SupunS @turbolent

@turbolent turbolent self-assigned this Jun 9, 2023
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

@turbolent turbolent merged commit 181319a into onflow:master Jun 9, 2023
@turbolent turbolent changed the title Add support for retrieving stdlib events [test] Add support for retrieving stdlib events Jun 9, 2023
@m-Peter m-Peter deleted the add-support-for-stdlib-events branch June 21, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants