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

lnd.py: add tests for and refactor create_wallet() #31

Merged
merged 12 commits into from
Aug 4, 2019

Conversation

ln2max
Copy link
Contributor

@ln2max ln2max commented Jul 17, 2019

Fix line lengths to pep8-compliant 79 chars, and break out constants from create_wallet() ahead of writing tests. This will make testing easier at a later stage (since we can reference module.CONSTANT_NAME in unit tests)

Please test this PR before merging, I have not got noma set up locally yet for "live" testing!

Generally accepted best practice is to store "magic numbers" and the
like as constants, which allows us to avoid repetition and errors, and
will make testing easier at a later stage (since we can reference
module.CONSTANT_NAME in unit tests)
@nolim1t
Copy link
Member

nolim1t commented Jul 18, 2019

Looks good but need someone to make sure it all works for the two scenarios at least.

  • Generate new wallet without seed
  • Generate wallet with a seed

@AnotherDroog
Copy link
Member

I’ll write tests before merging.

Thank you for your contribution!

@ln2max
Copy link
Contributor Author

ln2max commented Jul 19, 2019

I'm working on tests as well...

key thing is the last test which I haven't got working right
@ln2max
Copy link
Contributor Author

ln2max commented Jul 19, 2019

Added some preliminary tests:
https://github.com/ln2max/noma/blob/lnd_tests/tests/test_lnd.py

@nolim1t
Copy link
Member

nolim1t commented Jul 20, 2019

Good job with the tests!

That create wallet function was just originally a hack job from its humble beginnings. Glad to see theres some testing coverage in it.

@ln2max
Copy link
Contributor Author

ln2max commented Jul 21, 2019

Finished tests for create_wallet(), everything should work... now to start refactoring

@ln2max ln2max changed the title lnd.py: pep8 line lengths, break out constants from create_wallet() lnd.py: minor create_wallet() changes, add tests for create_wallet() Jul 21, 2019
this seems to be an issue with the order of imports, the tests appear to
no longer be correctly mocking requests.get() and requests.post()
see previous commit message for hypothesis (issue is the same)
@ln2max
Copy link
Contributor Author

ln2max commented Jul 23, 2019

Refactor done

@ln2max ln2max changed the title lnd.py: minor create_wallet() changes, add tests for create_wallet() lnd.py: add tests for and refactor create_wallet() Jul 23, 2019
@ln2max
Copy link
Contributor Author

ln2max commented Jul 23, 2019

(This PR still has not been tested on a running system)

@AnotherDroog
Copy link
Member

Now that there's a simple standalone neutrino mode, we can test more easily

@AnotherDroog AnotherDroog self-assigned this Aug 4, 2019
@AnotherDroog AnotherDroog added the enhancement New feature or request label Aug 4, 2019
@AnotherDroog AnotherDroog self-requested a review August 4, 2019 17:46
@AnotherDroog
Copy link
Member

Excellent work @ln2max!

Logic works flawlessly and code quality is greatly improved.

I have confirmed that keys get created and restored

@AnotherDroog AnotherDroog merged commit 0c42fa9 into lncm:master Aug 4, 2019
@AnotherDroog AnotherDroog mentioned this pull request Aug 23, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants