Skip to content

[QA] restructure rpc tests directory #6097

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 6 commits into from
May 27, 2015
Merged

[QA] restructure rpc tests directory #6097

laanwj merged 6 commits into from
May 27, 2015

Conversation

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented May 2, 2015

  • move non-test classes to subdir test-framework
  • added all test script into two sections to pull-tester/rpc-tests.sh
  • updated readme

@jonasschnelli jonasschnelli force-pushed the 2015/05/rpc_test_organizing branch 9 times, most recently from cd5991c to 9e4c1e0 Compare May 2, 2015 11:58
@laanwj laanwj added the Tests label May 4, 2015
@jonasschnelli jonasschnelli force-pushed the 2015/05/rpc_test_organizing branch from 9e4c1e0 to 2a87143 Compare May 6, 2015 12:36
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented May 6, 2015

rebased

@jonasschnelli jonasschnelli force-pushed the 2015/05/rpc_test_organizing branch from 2a87143 to 970bc51 Compare May 7, 2015 20:26
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented May 7, 2015

Fixed travis issue. Rebased.

@jonasschnelli jonasschnelli force-pushed the 2015/05/rpc_test_organizing branch 2 times, most recently from a0d81ba to 71b3156 Compare May 7, 2015 20:31
@sipa
Copy link
Member

sipa commented May 8, 2015

No opinion.

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented May 8, 2015

The benefits of this pull is not just another type of structure instead it's a clean distinction between test classes/files and utility classes/files.
At the moment you can't distinguish between a runnable test and a utility class within the folder ./qa/rpc-tests. Since #5981 it's even more mixed up. It's hard to get a overview of all possible test we have (thats why i extended rpc-test.sh).

Basically: it's a mess.

IMO messy qa/test setups end up in messy tests and bad test coverage. Therefore it should be clean at the root.
This simplifies the test setup and allow upcoming contributors to reduce the learn-curve of how a test should be written.

@sdaftuar
Copy link
Member

sdaftuar commented May 14, 2015

Concept ACK. I agree that the rpc-tests directory is a bit messy, and I have found it difficult in the past to just find all the tests that I can run. Making rpc-tests.sh be a canonical place to put new scripts so others know to run them seems like a helpful thing which I plan to use.

I don't have much opinion about how to best organize python code, but what you have done with the test_framework directory looks okay to me.

One thing I noticed -- I think script_test.py should probably be commented out of the extended test scripts. Because it's just comparing behavior, it really doesn't make sense to run except with two different bitcoind binaries, which we don't have a way of doing in an automated way right now. If the test were to give an error, it would almost certainly mean that the test is itself broken, and it's extremely slow to run.

@gavinandresen
Copy link
Contributor

gavinandresen commented May 14, 2015

concept ACK, tidier is better.

@theuni
Copy link
Member

theuni commented May 14, 2015

concept ACK, this was in need of some organization for sure.

@laanwj
Copy link
Member

laanwj commented May 18, 2015

Concept ACK, I like moving the utility functions to a python package so that the top-level files are runnable scripts (needs rebase, though).

@jonasschnelli jonasschnelli force-pushed the 2015/05/rpc_test_organizing branch 2 times, most recently from bc37204 to 27b9e4a Compare May 18, 2015 13:28
@jonasschnelli jonasschnelli force-pushed the 2015/05/rpc_test_organizing branch from 27b9e4a to 1629330 Compare May 18, 2015 13:29
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented May 18, 2015

Fixed @sdaftuar's nit (disabled script_test.py).
Rebased.

@jonasschnelli jonasschnelli force-pushed the 2015/05/rpc_test_organizing branch 2 times, most recently from 9324d4b to 2e35078 Compare May 19, 2015 08:43
@jonasschnelli jonasschnelli force-pushed the 2015/05/rpc_test_organizing branch from 2e35078 to a85b2e2 Compare May 19, 2015 14:41
@laanwj
Copy link
Member

laanwj commented May 21, 2015

It seems you forgot to update rpcbind_test.py for the new python-bitcoinrpc path.

Speaking of which - let's move the bitcoinrpc directory directly into rpc-tests so that the Add python-bitcoinrpc to module search path hackery (in four places!) is no longer needed and we can just do:

from bitcoinrpc.authproxy import AuthServiceProxy, JSONRPCException

... directly

@jonasschnelli jonasschnelli mentioned this pull request May 21, 2015
Closed
@jonasschnelli
rpc-tests: remove python-bitcoinrpc directory
7b7f258 
place authproxy.py at same level as other utility classes
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented May 21, 2015

Totally agreed.
I just remove the python-bitcoinrpc completely and placed the authproxy.py file directly into the new test-framework folder (it's not a submodule so it can be treated as normal utility class).
Also fixed the rpcbind_test.py as mentioned by @laanwj

@laanwj laanwj merged commit 7b7f258 into bitcoin:master May 27, 2015
laanwj added a commit that referenced this pull request May 27, 2015
@laanwj
Merge pull request #6097
c608b6d 
7b7f258 rpc-tests: remove python-bitcoinrpc directory (Jonas Schnelli)
a85b2e2 pull-tester/rpc-tests.sh: disable script_test.py test (Jonas Schnelli)
3e875b1 pull-tester/rpc-tests.sh: allow passing throug of arguments (Jonas Schnelli)
00706a5 update rpc-tests readme.md (Jonas Schnelli)
344e08e extend rpc-tests.sh control script with non-travis tests (Jonas Schnelli)
64937fe [QA] restructure rpc tests directory (Jonas Schnelli)
@laanwj
Copy link
Member

laanwj commented May 27, 2015

@jonasschnelli Nice solution

laanwj added a commit that referenced this pull request Jul 2, 2015
@laanwj
tests: fix txn_clone.py
3f16971 
Solve merge conflict of test added in #5881 with #6097.
@maflcko maflcko mentioned this pull request Nov 18, 2015
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jonasschnelli @sipa @sdaftuar @gavinandresen @theuni @laanwj