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

Move non-mempool specific functions to new file. #525

Merged
merged 1 commit into from
Oct 27, 2015
Merged

Move non-mempool specific functions to new file. #525

merged 1 commit into from
Oct 27, 2015

Conversation

dajohi
Copy link
Member

@dajohi dajohi commented Oct 22, 2015

No functional change. Add tests.

Review on Reviewable

@davecgh
Copy link
Member

davecgh commented Oct 22, 2015

As discussed on IRC, I like the concept.

// TestCalcMinRequiredTxRelayFee tests the calcMinRequiredTxRelayFee API.
func TestCalcMinRequiredTxRelayFee(t *testing.T) {
tests := []struct {
size int64 // Transaction size in bytes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please give the tests a name string field and include it in the error output. I know there are several tests that don't do that currently, but we've been trying to move that direction since it's much harder to figure out which test failed when only given a number. Granted it's not too difficult when there are only a few tests, but I'd like to be consistent.

@davecgh
Copy link
Member

davecgh commented Oct 23, 2015

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


policy.go, line 27 [r1] (raw file):
Please don't wrap function definitions.


Comments from the review on Reviewable.io

@davecgh
Copy link
Member

davecgh commented Oct 26, 2015

Reviewed 1 of 2 files at r2.
Review status: 2 of 3 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.


policy_test.go, line 72 [r2] (raw file):
This looks leftover. Please use t.Fatalf.


policy_test.go, line 79 [r2] (raw file):
Why not just make this *txscript.ScriptBuilder and avoid the extra indirects below?


policy_test.go, line 158 [r2] (raw file):
Better error message please. This won't be very useful if it gets hit due to an invalid test case.


policy_test.go, line 198 [r2] (raw file):
Please put the actual relay fee that is being tested, not the default. If you want to prove the default hasn't changed, then add another test like func TestDefaultMinRelayFeePolicy(t *testing.T) {...}.


policy_test.go, line 204 [r2] (raw file):
Same.


policy_test.go, line 210 [r2] (raw file):
Same.


policy_test.go, line 217 [r2] (raw file):
Same.


policy_test.go, line 224 [r2] (raw file):
Same.


policy_test.go, line 232 [r2] (raw file):
Same.

Also, please use a few different values. Only testing 0 and 0.00001 doesn't really exercise the function very well.


Comments from the review on Reviewable.io

No functional change. Add tests.
@davecgh
Copy link
Member

davecgh commented Oct 27, 2015

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@davecgh
Copy link
Member

davecgh commented Oct 27, 2015

OK

@conformal-deploy conformal-deploy merged commit 3d6afcf into btcsuite:master Oct 27, 2015
@dajohi dajohi deleted the organize branch October 27, 2015 16:18
davecgh pushed a commit to davecgh/btcd that referenced this pull request Dec 23, 2016
All corner cases are thoroughly documented in the Generate function in
file blockchain/stakeversiontests/generate.go.

Fixes btcsuite#525
jcvernaleo pushed a commit to jcvernaleo/btcd that referenced this pull request Dec 23, 2016
All corner cases are thoroughly documented in the Generate function in
file blockchain/stakeversiontests/generate.go.

Fixes btcsuite#525
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.

3 participants