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

Update NeoTestRunner usage in the documentation #1096

Merged
merged 2 commits into from
Jul 18, 2023
Merged

Conversation

meevee98
Copy link
Contributor

No description provided.

@meevee98 meevee98 requested a review from luc10921 July 14, 2023 18:46
@meevee98 meevee98 self-assigned this Jul 14, 2023
@lllwvlvwlll
Copy link
Member

Comment on lines 59 to 60
function you'll need a NeoTestRunner object, which requires the path of your Neo-Express network configuration file to
the NeoTestRunner object to set up the test environment.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function you'll need a NeoTestRunner object, which requires the path of your Neo-Express network configuration file to
the NeoTestRunner object to set up the test environment.
function you'll need a NeoTestRunner object, which takes the path of your Neo-Express network configuration file as an argument to set up the test environment.

@@ -51,9 +51,16 @@ Install [Neo-Express](https://github.com/neo-project/neo-express#installation) a

### Testing

Before writing your tests, make sure you have a Neo-Express network for local tests.
Please refer to [Neo-Express documentation](https://github.com/neo-project/neo-express/blob/master/docs/command-reference.md#neoxp-create)
Copy link
Member

Choose a reason for hiding this comment

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

I kind of feel we could give just enough information that they don't have to first starting reading another link. Something like

"If you do not yet have a local network, open a terminal and type neoxp create. Please refer to the Neo-Express documentation for more details on how to configure your local network."

From here they can choose to fine tune their network or just go ahead

function you'll need a NeoTestRunner object, which requires the path of your Neo-Express network configuration file to
the NeoTestRunner object to set up the test environment.

You'll to call the method `call_contract()`. Its parameters are the path of the compiled smart contract, the smart
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You'll to call the method `call_contract()`. Its parameters are the path of the compiled smart contract, the smart
You'll have to call the method `call_contract()` to interact with your smart contract. Its parameters are the path of the compiled smart contract, the smart

project_root_folder = '{path-to-project-root-folder}'
path = f'{project_root_folder}/boa3_test/examples/hello_world.nef'
runner = NeoTestRunner(neoxp_folder)
runner = NeoTestRunner(neoxp_config_file)

invoke = runner.call_contract(path, 'main')
runner.execute()
Copy link
Member

Choose a reason for hiding this comment

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

This apparently can return an error message. If it does, then inspecting invoke.result is going to be really confusing. For example, I was calling a wrong function (=wrong name, didn't exist). What invoke.result shows is
image

I wanted to add the comment that we should promote error check prior to asserting the results. However, that is going to be somewhat hard because the return type is something like Union[str, list[NeoInvokeResult]] where str is an exception message. So that means one has to do something like

result = runner.execute()
if isinstance(result, str):
   raise Exception(result)
assert invoke.result is None

What I personally think would have been prettier is

success, err_msg = runner.execute()
if not success:
   raise Exception(err_msg)
assert invoke.result is None

Alternatively (and maybe even better) is that runner.execute() throws an exception if it fails. Under the assumption that we eventually move to a unittest like class you could then do scenarios like

with self.assertRaises(ValueError) as context:
   runner.execute()
self.assertIn("method not found", str(context.exception))

and maybe even nicely handle in contract asserts (if this PR ever gets accepted)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Boa3 tests, we always check if the VMState on the runner is the expected (HALT if the execution is expected to succeed, FAULT if it's expected to fail).
For now, we could recommend this on the documentation as well.

@meevee98 meevee98 requested a review from ixje July 17, 2023 14:41
@coveralls
Copy link
Collaborator

coveralls commented Jul 17, 2023

Coverage Status

coverage: 91.611% (-0.3%) from 91.88% when pulling de0f254 on CU-8678c423x into d7d8e9f on development.

@luc10921 luc10921 merged commit 3b158cf into development Jul 18, 2023
@luc10921 luc10921 deleted the CU-8678c423x branch July 18, 2023 12:28
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