[refactor-prompt] Add missing tests for wallet create, verbose and create_addr

[LysanderGG]

What current issue(s) does this address, or what feature is it adding?
Add missing tests after PR #720 (also see #726 (comment))

The tests are for the following commands wallet create, wallet verbose, wallet create_addr.

How did you solve this problem?
Added some tests.

How did you make sure your solution works?
Run make test.

Are there any special changes in the code that we should be aware of?
No.

Please check the following, if applicable:

• Did you add any tests?
• Did you run make lint?
• Did you run make test?
• Are you making a PR to a feature branch or development rather than master?
• Did you add an entry to CHANGELOG.rst? (if not, please do)

Note:

• I couldn't find any other test related to wallet migration so I was unable to test the command wallet migrate.

[coveralls]

Coverage increased (+0.3%) to 83.838% when pulling 9a7f665 on LysanderGG:add_command_wallet_tests into c4bb448 on CityOfZion:refactor-prompt.

[jseagrave21]

This looks good. Could you also add a test for wallet?

[jseagrave21]

Also, it would be nice to see a test for an invalid wallet command

[jseagrave21]

And, I was looking here: https://coveralls.io/builds/20405190/source?filename=neo/Prompt/Commands/Wallet.py#L109
Is the logic to trigger this effect wrong, or maybe the previous returns premature? It seems like we have nothing actually triggering start_wallet_loop
Maybe something like

...
            try:
                PromptData.Wallet = UserWallet.Create(path=path, password=password_key)
                contract = PromptData.Wallet.GetDefaultContract()
                key = PromptData.Wallet.GetKey(contract.PublicKeyHash)
                print("Wallet %s" % json.dumps(PromptData.Wallet.ToJson(), indent=4))
                print("Pubkey %s" % key.PublicKey.encode_point(True))
                # return PromptData.Wallet
            except Exception as e:
                print("Exception creating wallet: %s" % e)
                PromptData.Wallet = None
                if os.path.isfile(path):
                    try:
                        os.remove(path)
                    except Exception as e:
                        print("Could not remove {}: {}".format(path, e))
                return

            if PromptData.Wallet:
                PromptData.Prompt.start_wallet_loop()
                return PromptData.Wallet

Where the commented return PromptData.Wallet would just be deleted.

[ixje]

regarding migrate tests. I've never used the command before. I shall check if the command is still useful and otherwise we can remove it. It might be really old.

As for the remaining input; @jseagrave21 pointed them all out (thanks!)

1. test for wallet (coverage)
2. test invalid wallet (coverage)
3. ensure wallet loop is actually started when creating a new wallet (functionality)

[LysanderGG]

@ixje @jseagrave21 Thank you for the first review!

1. test for wallet (coverage)
   -> Done, but there's not much to test.
   Shall I test what is printed to std::out?

2. test invalid wallet (coverage)
   -> Done, but there's not much to test.

3. ensure wallet loop is actually started when creating a new wallet (functionality)
   The bug had been introduced with Add support for wallet send, sendmany, sign, open, and close #726.
   I fixed it.

Please review a second time when you have time.

[ixje]

1. could be ok to do that. Just to make sure wallet is actually printing wallet contents. Or perhaps patch and assert that wallet.ToJson() is called
2. not working as intended, see review
3. thanks!

[ixje]

same as previous. This does not test verbose, but fails on a not having an open wallet.

[LysanderGG]

This test is specifically testing test wallet verbose with no wallet

[ixje]

not specifically at this line, but in the function it calls (not seeing that code here);
Can you please give a more user friendly error message and/or capture the "not enough arguments" problem sooner? We now get

neo> wallet create_addr
int() argument must be a string, a bytes-like object or a number, not 'NoneType'

which is really confusing because you might try interpret this as it indicating that "create_addr" is read as an int() argument

[LysanderGG]

Done here:

neo-python/neo/Prompt/CommandBase.py

Line 76 in 9a7f665

if len(arguments) < sub_cmd.nb_mandatory_params():

However the early detection makes code like

neo-python/neo/Prompt/Commands/Wallet.py

Line 113 in 9a7f665

print("Please specify a path")

unreachable.

[LysanderGG]

Amend of ^
I removed the early detection and checked the argument in CommandWalletCreateAddress.

[ixje]

I couldn't find any other test related to wallet migration so I was unable to test the command wallet migrate

Skip it. I'm going to remove all migrate code. It's old an no longer used.

[LysanderGG]

@ixje @jseagrave21 Thank you for the feedbacks.
I fixed what was requested, please check it. There were a few different suggestions for some tests so I chose one.

[jseagrave21]

I like the new logic but it looks like several lines were uncovered which is undesirable imo.

[jseagrave21]

Moved this review closer to the subject lines

[LysanderGG]

@ixje @jseagrave21
I pushed a new commit d8d0b04
Please have a look.

I changed the tests to use the subcommand directly instead of testing through CommandWallet. I think it's better for the unit tests to use the smaller unit possible.

If we want integration tests, we should probably test from the prompt directly instead of CommandWallet that is kind of in between.

Please let me know what you think.

Note: testing with the subcommands allows to cover all the code. I added a some edge case detection in the subcommand's execute() method where necessary.
In practice, when called through CommandWallet it is detected earlier.

[jseagrave21]

@LysanderGG I prefer integrated tests which simulate user input as much as possible. When I implemented the testing method here #726 (comment) I wasn't sure how to test directly from prompt.py. But if that is possible, I would prefer that.

[ixje]

Back here #726 (comment) I kind of reasoned that testing "GroupCommand" vs "SubCommand" doesn't really matter, although "GroupCommand" makes it more obvious what scenario we're working on and will need testing anyway. Which would be in line with @jseagrave21 's last comment.

After your discussion above I kind of realise that people might start calling commands directly when they're extending neo-python. e.g. when they implement a web interface, REST API, new RPC commands or want to control neo-python programmatically. They should not have to re-create the logic for let's say sending assets when there's a CommandSend already doing this. Now being aware of this I don't think we can avoid some code duplication between the "Group Command" and "SubCommand" so to say. For the extensibility reasons mentioned before, I would now lean towards testing commands directly while not excluding testing group commands. (let me know if that last line doesn't make sense and I'll try to rephrase)

[LysanderGG]

Sorry for all the back and forth.

So, here is what comes with the latest commits:

• I removed the early missing arguments detection. Each function needs to validate its arguments so that we can reuse them.
• I reverted the change of tests to SubCommands, it's back to using CommandWallet for tests.
• All lines of Wallet.py are covered, except for Migrate (https://coveralls.io/jobs/42907770/source_files/431067693)

I think that the best to do is keep the functionnalities not related to the command line outside of Commands so that these functions can be used elsewhere.
Commands and SubCommands should not be used from outside (not what I said before, I know 😄 ). Instead, the functions they call can be called (example CreateAddress https://github.com/CityOfZion/neo-python/blob/refactor-prompt/neo/Prompt/Commands/Wallet.py#L220 and fonctions after, or construct_send_basic https://github.com/CityOfZion/neo-python/blob/refactor-prompt/neo/Prompt/Commands/Send.py#L91 and other functions after).

With this we can test both the functions themselves (unit tests like) and the integration in the prompt command. Through root commands should be fine.

I'm a bit lost with this big discussion; can you

1. Confirm the unresolved conversations?
2. review again and add new comments if something is wrong or left to be done for this PR?

Thank you!

[ixje]

I think that the best to do is keep the functionnalities not related to the command line outside of Commands so that these functions can be used elsewhere.

I actually had that in mind (separate presentation from functionality) before writing my previous comment, but thought it might lead to more confusion at this point.

I'm a bit lost with this big discussion; can you
No worries, same here. This is a good showcase why we should keep PR's small, because discussions can still get big :)

I think for this one let's get it in and we'll think about moving out the body of execute() functions before we do the big merge. That way we can have it in a clean PR which is easier to discuss. @jseagrave21 any objections at this point or things that need solving before merging?

PS: I added the last paragraph as a TODO item to the original "CLI refactoring post"

[jseagrave21]

@ixje @LysanderGG everything looks good to me! Good work!