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

switched current tests db to memdb, added memdb iterator #469

Merged
merged 6 commits into from Jan 27, 2019

Conversation

alonp99
Copy link
Contributor

@alonp99 alonp99 commented Jan 22, 2019

#441

  • switched leveldb usage in test to memdb
  • added iterator for memdb

some code rely on leveldb iterator so I had to implement iterator for memdb with the same interface

@alonp99 alonp99 changed the title switched current tests db to memdb, added memdb iterator for leveldb … switched current tests db to memdb, added memdb iterator Jan 22, 2019
database/db_test.go Outdated Show resolved Hide resolved
@almogdepaz
Copy link
Contributor

@alonp99 in syncer_test.go please add an integration test that uses level db.
should be similar to what TestSyncProtocol_SyncMultipleNodes looked like before your changes
please use

if testing.Short() {
t.Skip()
}

in the start of the test

package database

import (
"github.com/syndtr/goleveldb/leveldb/util"
Copy link
Contributor

Choose a reason for hiding this comment

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

try to avoid coupling memory database to level_db
you can declare a new iterator interface if it helps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, but I had to do it because database.DB expose leveldb iterator type.
Let me know if you have an idea how to tackle it

Copy link
Contributor

Choose a reason for hiding this comment

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

just declare a releaser interface in that package and use it instead

Copy link
Contributor

Choose a reason for hiding this comment

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

We are only using 2 functions for iteration, next() and release, you can create an interface containing only these functions. the levelDB iterator will implement these as well

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 for the suggestions, made a new "base" iterator interface

@alonp99
Copy link
Contributor Author

alonp99 commented Jan 25, 2019

@alonp99 in syncer_test.go please add an integration test that uses level db.
should be similar to what TestSyncProtocol_SyncMultipleNodes looked like before your changes
please use

if testing.Short() {
t.Skip()
}

in the start of the test

@almogdepaz done, please have a look

@almogdepaz
Copy link
Contributor

@alonp99 some of your tests are failing, other then that the changes look fine

@antonlerner antonlerner merged commit 58b6bba into spacemeshos:develop Jan 27, 2019
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.

None yet

5 participants