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

Improve account related test coverage #239

Merged
merged 1 commit into from
Apr 27, 2015

Conversation

tuxcanfly
Copy link
Contributor

Added test coverage for account related manager funcs.

used: true,
tests := []struct {
name string
addrType btcutil.Address
Copy link
Member

Choose a reason for hiding this comment

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

Is this field needed? doesn't look used and the address in the p2sh address test is actually a p2pkh addr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using this field now, to switch between managedAddress and scriptAddress so that both paths are covered in the test cases.

Copy link
Member

Choose a reason for hiding this comment

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

The type itself is used, but not the data. I would consider using constants here instead:

func testMarkUsed(tc *testContext) bool {
    type addrType byte
    const (
        addrPubKeyHash addrType = iota
        addrScriptHash
    )
    tests := []struct {
        typ addrType
        // ...
    }

    // ...

        switch test.typ {
        case addrPubKeyHash:
            // ...
        case addrScriptHash:
            // ...
        default:
            panic("unreachable")
        }

    // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated to use addrType const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, removed it.

@tuxcanfly tuxcanfly force-pushed the accounts-coverage branch 8 times, most recently from 72983f2 to 7ddf007 Compare April 17, 2015 12:41
@@ -53,6 +53,149 @@ var (

// waddrmgrNamespaceKey is the namespace key for the waddrmgr package.
waddrmgrNamespaceKey = []byte("waddrmgrNamespace")

// expectedExternalAddrs is the list of expected external addresses
Copy link
Member

Choose a reason for hiding this comment

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

Please remove tab characters between the // and the comment text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, slipped past my editor, removed.

@tuxcanfly tuxcanfly force-pushed the accounts-coverage branch 7 times, most recently from a1ba83d to 9b42268 Compare April 24, 2015 18:11

const (
addrPubKeyHash addrType = iota
addrPubKey addrType = iota
Copy link
Member

Choose a reason for hiding this comment

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

Unneeded type and iota here

@jrick
Copy link
Member

jrick commented Apr 27, 2015

This looks ok now. Can you squash and rebase?

@tuxcanfly
Copy link
Contributor Author

@jrick Thanks, rebased now.

@davecgh
Copy link
Member

davecgh commented Apr 27, 2015

OK

@davecgh
Copy link
Member

davecgh commented Apr 27, 2015

Need rebase to latest master.

@tuxcanfly
Copy link
Contributor Author

OK, squashed and rebased to latest master.

@conformal-deploy conformal-deploy merged commit ee72c81 into btcsuite:master Apr 27, 2015
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.

4 participants