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

Minor RPC Test updates #804

Merged
merged 2 commits into from
Jun 27, 2019
Merged

Minor RPC Test updates #804

merged 2 commits into from
Jun 27, 2019

Conversation

nodech
Copy link
Member

@nodech nodech commented Jun 26, 2019

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.

@nodech nodech requested a review from pinheadmz June 26, 2019 14:21
@codecov-io

This comment has been minimized.

@braydonf
Copy link
Contributor

braydonf commented Jun 26, 2019

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).

@braydonf braydonf added the ready for review Ready to be reviewed label Jun 26, 2019
@pinheadmz
Copy link
Member

LGTM, tests pass locally. Code coverage is the same as master PLUS the bonus coverage of wallet RPC getwalletinfo 👍

@tynes
Copy link
Member

tynes commented Jun 26, 2019

I started test/wallet-rpc-test.js so that each RPC method could have its own describe, we probably want to be sure to keep things in a similar style if we are adding to that file

@nodech
Copy link
Member Author

nodech commented Jun 26, 2019

@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.

@nodech
Copy link
Member Author

nodech commented Jun 26, 2019

@braydonf yeah, that's unfortunate. That's one of the reasons I did this early to other rpc PRs so we can avoid rapid rebases on other pending PRs. My other PR #802 will need rebase as well.

before(async () => {
consensus.COINBASE_MATURITY = 0;
Copy link
Contributor

@braydonf braydonf Jun 27, 2019

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.

Copy link
Contributor

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.

Copy link
Member Author

@nodech nodech Jun 27, 2019

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

Copy link
Contributor

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.

Copy link
Contributor

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.

@nodech
Copy link
Member Author

nodech commented Jun 27, 2019

@braydonf okay, pushed your proposed changes to the walletrpc. thanks

@nodech nodech force-pushed the rpc-cleanup branch 7 times, most recently from 272c173 to 575448e Compare June 27, 2019 18:21
@braydonf
Copy link
Contributor

In case anyone runs into unusually slow CI tests again (as experienced here), it's resolved by specifiying the workerSize (background details at #777). It's related to nyc and there being far too many workers by default as the CI environment has many cores.

@braydonf braydonf merged commit c7605ac into bcoin-org:master Jun 27, 2019
braydonf added a commit that referenced this pull request Jun 27, 2019
@nodech nodech deleted the rpc-cleanup branch June 28, 2019 08:25
@braydonf braydonf removed the ready for review Ready to be reviewed label Jul 3, 2019
@braydonf braydonf mentioned this pull request Oct 8, 2019
@braydonf braydonf added this to the 2.0.0 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants