-
Notifications
You must be signed in to change notification settings - Fork 808
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
Minor RPC Test updates #804
Conversation
This comment has been minimized.
This comment has been minimized.
Noticed that as well, very few node rpc tests, as rpc-test is actually mostly wallet tests, good to have that organized. This will conflict with #605 and it will need to be rebased yet again (currently needs rebase). |
LGTM, tests pass locally. Code coverage is the same as master PLUS the bonus coverage of wallet RPC |
I started |
@tynes That's the plan, when I add more test cases to each of the calls it will become describe and more isolated (so you can run tests separately). This is kinda first step to that. |
test/wallet-rpc-test.js
Outdated
before(async () => { | ||
consensus.COINBASE_MATURITY = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a chance this isn't restored if any of the below fail within this test, it'll probably be preferable to quickly generate more than 100 blocks, at the start of the tests and avoid modifying globals, or at-least do so carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be another follow-up that is necessary, there may be other such cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this can happen if before
fails and can affect other test cases.
Ideally consensus should be accepted as parameters in places where it's necessary instead of being used as globals. That would make testing much easier and more flexible.
Generating 100 blocks is not too much to ask but slows downs generation quite a lot, in case we have many separate test cases it may become burden. We could have "mock" chain that already has 100 blocks in one way or another backed up in test/data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generating 200 blocks takes around 400-180ms, as from some other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's done a lot, it could add up, but generally there can be setup scripts that build out a chain, and many tests that run against it.
@braydonf okay, pushed your proposed changes to the walletrpc. thanks |
272c173
to
575448e
Compare
In case anyone runs into unusually slow CI tests again (as experienced here), it's resolved by specifiying the |
Move wallet rpc tests to its own file.
Clean up some test cases.
There is only minor logic clean up done in this PR.
Upcoming PRs will try to make each test case as independent as possible as well.