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

test: 81% coverage on blockstore #3074

Merged
merged 3 commits into from
Aug 18, 2016
Merged

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Aug 11, 2016

Coverage report available at: https://ipfs.io/ipfs/QmTuMtwGCfHrbYyZdQ1RaGNwS2MGsmAkjA8AaB69N7Ya1g/coverage.html#file0

Part of #3053

License: MIT
Signed-off-by: Jakub Sztandera kubuxu@protonmail.ch

@Kubuxu
Copy link
Member Author

Kubuxu commented Aug 11, 2016

Docker tests failed but it isn't connected.

@Kubuxu Kubuxu force-pushed the feat/test-cover-blockstore branch 2 times, most recently from 44ef280 to c40926c Compare August 15, 2016 14:20
Coverage report available at: https://ipfs.io/ipfs/QmTuMtwGCfHrbYyZdQ1RaGNwS2MGsmAkjA8AaB69N7Ya1g/coverage.html#file0

Part of #3053

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@ipfs.io>
@Kubuxu Kubuxu added the need/review Needs a review label Aug 16, 2016
@@ -57,16 +57,21 @@ func TestRuntimeHashing(t *testing.T) {
bs := NewBlockstore(ds_sync.MutexWrap(ds.NewMapDatastore()))
bl := blocks.NewBlock([]byte("some data"))
blBad, err := blocks.NewBlockWithHash([]byte("some other data"), bl.Key().ToMultihash())
bl2 := blocks.NewBlock([]byte("some other data"))
Copy link
Member

Choose a reason for hiding this comment

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

why is this here before the error check from the previous statement?

Copy link
Member Author

Choose a reason for hiding this comment

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

because it should always success and it is checked in other tests, will add check either way.

Copy link
Member

Choose a reason for hiding this comment

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

still, you want to check errors right as they are created, otherwise it just looks sloppy.

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Copy link
Member Author

Kubuxu commented Aug 16, 2016

Updated.

opts.HasARCCacheSize = -1

if _, err := CachedBlockstore(nil, nil, opts); err == nil {
t.Error("wrong ARC setting was not detected")
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for Error over Fatal?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those test cases are not dependent on each other, so using Error all test cases will checked and printed.

Error = Print + Fail
Fatal = Print + FailNow

Copy link
Member

Choose a reason for hiding this comment

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

I guess thats fair.

@whyrusleeping
Copy link
Member

just a typo then LGTM

Also format imports

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Copy link
Member Author

Kubuxu commented Aug 18, 2016

Done.

@whyrusleeping whyrusleeping merged commit 1a4361e into master Aug 18, 2016
@whyrusleeping whyrusleeping deleted the feat/test-cover-blockstore branch August 18, 2016 18:02
@ghost ghost mentioned this pull request Dec 23, 2016
@momack2 momack2 added this to Needs Review in ipfs/go-ipfs May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
No open projects
ipfs/go-ipfs
Needs Review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants