[tests] Make rpcauth.py testable and add unit tests by nixbox · Pull Request #13056 · bitcoin/bitcoin

nixbox · 2018-04-23T07:47:56Z

laanwj · 2018-04-23T14:59:14Z

Some lint issues:

./test/util/rpcauth-test.py:19:1: E402 module level import not at top of file
./test/util/rpcauth-test.py:20:1: E402 module level import not at top of file
./test/util/rpcauth-test.py:21:1: E402 module level import not at top of file
./test/util/rpcauth-test.py:22:1: E402 module level import not at top of file
^---- failure generated from contrib/devtools/lint-python.sh

Also the test fails to launch:

/usr/bin/python3.4 ../test/util/rpcauth-test.py
/usr/bin/python3.4: can't open file '../test/util/rpcauth-test.py': [Errno 2] No such file or directory

nixbox · 2018-04-24T01:27:07Z

Fixed the lint issues.

Fixed the "No such file or directory" problem as well.

laanwj · 2018-04-24T12:09:52Z

utACK ebff14786b7addd4bcc6e74691394a77019f2bf6

jnewbery

A few nits inline after testing.

jnewbery · 2018-04-24T15:00:27Z

nit: seems like somewhat random placement. Why not alphabetically sorted like the rest?

jnewbery · 2018-04-24T15:02:46Z

nit: prefer _ for unused loop variables.

jnewbery · 2018-04-24T15:03:18Z

nit: prefer docstrings for function-level comments.

jnewbery · 2018-04-24T15:12:12Z

No need to use parens here. Just:

password, password_hmac = generate_password(salt)

(you could also change the return statement in generate_password() to return password, password_hmac, but under the covers Python will construct a tuple to return anyway, so it's equivalent.)

jnewbery · 2018-04-24T15:14:48Z

nit: prefer string formatters ({} or % - we can't use f-strings yet).

If you must concatenate strings, please surround the + with spaces so the code isn't so difficult to parse.

jnewbery · 2018-04-24T15:16:16Z

I think this is unnecessary - this is for python2 compatibility. We now no longer support python2 across the project.

jnewbery · 2018-04-24T15:23:58Z

nit: prefer with open() as: for opening files.

nixbox · 2018-04-24T17:18:25Z

Mistakenly did a force push. Addressed all of your nits.

Thanks for the feedback!

jnewbery

couple more nits inline

jnewbery · 2018-04-24T17:26:26Z

Docstrings should be the first statement inside the function (https://www.python.org/dev/peps/pep-0257/#what-is-a-docstring)

jnewbery · 2018-04-24T17:28:14Z

nit: use os.path.join() for maximum portability (instead of concat'ing path strings)

Also, should probably say config_path

maflcko · 2018-04-24T18:14:33Z

Could do a rebase, since this is already in master?

refs #12995

jnewbery · 2018-04-24T19:15:49Z

Tested ACK 6674a75.

Travis failure was unrelated failure in mining_prioritisetransaction.py. I've restarted the job.

nixbox · 2018-04-25T00:07:18Z

All green, ready to go?

6674a75 [tests] Make rpcauth.py testable and add unit tests (Qasim Javed) Pull request description: refs #12995 Tree-SHA512: 609a85a75dafe46723f050db1e96098f6859519afd5e1b141a332f34ab715144d32fdd8f10d245d6dc0765e77f82158fe7b8aa2e4317f0e893de491d2e0acf1f

Add rpcauth-test (introduced bitcoin#13056) to AC_CONFIG_LINKS, like the other directly called python scripts, to fix out-of-tree `make check`.

…-tree make check 3e53004 test: add rpcauth-test to AC_CONFIG_LINKS to fix out-of-tree make check (Wladimir J. van der Laan) Pull request description: Add rpcauth-test (introduced #13056) to AC_CONFIG_LINKS, like the other directly called python scripts, to fix out-of-tree `make check`. (forgot to test this before merging, unfortunately) Tree-SHA512: 60306ac83ee81e0f27d5b4f0420c3bd4edfa3fd5daaa067ff0c235eb55da9f6c559203c2625fed97783b2d11d9f1bed7a359b4d9204ab4af75fd1fd24b9882a8

Add rpcauth-test (introduced bitcoin#13056) to AC_CONFIG_LINKS, like the other directly called python scripts, to fix out-of-tree `make check`.

6674a75 [tests] Make rpcauth.py testable and add unit tests (Qasim Javed) Pull request description: refs bitcoin#12995 Tree-SHA512: 609a85a75dafe46723f050db1e96098f6859519afd5e1b141a332f34ab715144d32fdd8f10d245d6dc0765e77f82158fe7b8aa2e4317f0e893de491d2e0acf1f

… out-of-tree make check 3e53004 test: add rpcauth-test to AC_CONFIG_LINKS to fix out-of-tree make check (Wladimir J. van der Laan) Pull request description: Add rpcauth-test (introduced bitcoin#13056) to AC_CONFIG_LINKS, like the other directly called python scripts, to fix out-of-tree `make check`. (forgot to test this before merging, unfortunately) Tree-SHA512: 60306ac83ee81e0f27d5b4f0420c3bd4edfa3fd5daaa067ff0c235eb55da9f6c559203c2625fed97783b2d11d9f1bed7a359b4d9204ab4af75fd1fd24b9882a8

fanquake added the Tests label Apr 23, 2018

jnewbery reviewed Apr 24, 2018

View reviewed changes

maflcko reviewed Apr 24, 2018

View reviewed changes

[tests] Make rpcauth.py testable and add unit tests

6674a75

refs #12995

laanwj merged commit 6674a75 into bitcoin:master Apr 25, 2018

laanwj mentioned this pull request Apr 25, 2018

test: add rpcauth-test to AC_CONFIG_LINKS to fix out-of-tree make check #13073

Merged

Bushstar mentioned this pull request Apr 26, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#309

Merged

SmarterHomes mentioned this pull request Sep 6, 2018

test: add rpcauth-test to AC_CONFIG_LINKS to fix out-of-tree make check chaincoin/chaincoin#263

Merged

dongcarl mentioned this pull request Nov 19, 2018

Integration tests that use rpcauth.py #14758

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Uh oh!

Conversation

nixbox commented Apr 23, 2018

Uh oh!

laanwj commented Apr 23, 2018

Uh oh!

nixbox commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Apr 24, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixbox Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixbox commented Apr 24, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 24, 2018

Uh oh!

nixbox commented Apr 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nixbox commented Apr 24, 2018 •

edited

Loading

nixbox Apr 24, 2018 •

edited

Loading