-
Notifications
You must be signed in to change notification settings - Fork 288
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
multi: add getwork tests. #2306
Conversation
b056d61
to
4d46f20
Compare
4d46f20
to
a86d111
Compare
a86d111
to
0b31e63
Compare
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.
The interface pieces look good now, but I see that the test coverage is missing several paths and submission altogether.
$ go test -run=TestHandleGetWork -coverprofile=cov.out
$ go tool cover -func=cov.out | grep -E handleGetWork
github.com/decred/dcrd/internal/rpcserver/rpcserver.go:3290: handleGetWorkRequest 92.7%
github.com/decred/dcrd/internal/rpcserver/rpcserver.go:3394: handleGetWorkSubmission 0.0%
github.com/decred/dcrd/internal/rpcserver/rpcserver.go:3482: handleGetWork 93.3%
0b31e63
to
e1eecf7
Compare
All paths not covered could not be hit.
Based on that I'd suggest removing the error check here since |
e1eecf7
to
8c52bc0
Compare
Regarding your comment about removing the unexpected error type check, I agree that it's currently impossible, and hence can't be tested, but I also know that code gets refactored and thus assumptions that like always end up broken over time. As a case in point, I see you already caught a case where it was checking for the wrong type of error. In this path, if the code gets refactored and some other issue happens, it would be completely hidden if the error check is removed as it would just look a miner submitting a block that didn't meet the work requirements even though it could be a real issue. |
8c52bc0
to
6236699
Compare
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.
Looks good. Super minor but I did notice that there is a typo in the commit message (This fixes a proof of work error check and adds rpc handler tests for getWork.)
This fixes a proof of work error check and adds rpc handler tests for getWork.
6236699
to
111a895
Compare
This adds rpc handler tests for
getwork
.Work towards #2069.