test: Test node pkg constructor via integration test suite #2641
Conversation
Will be used by our tests shortly, can also be used by users at somepoint too IMO.
Instead of the db constructor directly - before it was bypassing the main public (embedded) entry point to our codebase leaving it untested. I also think the new test code is slightly nicer. The switch statement was copied from the `NewBadgerFileDB` function above.
Was a bit annoying to get rid of, and easy to simplify
Was a bit annoying to get rid of, and easy to simplify
a805d6c to
ae8bf9d
Compare
fredcarle
left a comment
There was a problem hiding this comment.
Good idea to move to the new constructor. Just one thought to remove a bit of confusion from the added StoreOption field.
| type StoreOptions struct { | ||
| path string | ||
| inMemory bool | ||
| defraStore bool |
There was a problem hiding this comment.
thought: inMemory and defraStore are also synonyms. Maybe we could do this instead.
type Datastore int
const (
BadgerFile Datastore = iota
BadgerMemory
DefraMemory
)
type StoreOption struct {
...
store Datastore
}This way by default it will be BadgerFile and if we add more option we won't need more struct fields and we remove the memory store confusion.
There was a problem hiding this comment.
I was tempted by that initially, but I felt defra-store was more analogous to badger-store, and thus (at least long-term) both inMemory and path would be applicable to both, is just that defra-store has no file based implementation yet (and so also atm, always assumes inMemory is true).
If/when we support more stores, defraStore should be replaced by an enum that contains other store implementations (defra, badger, turtle, etc, etc), all of which can respect path and inMemory (saves multiplying the number of db-types)
There was a problem hiding this comment.
That leaves a point of confusion thought as turtle or others might not support in memory store but the option would still be available. So leaving it as is in the short term creates confusion between the badger in memory and defra in memory options and in the long term leaves in an option that might be irrelevant for some stores. I do have a preference to remove the confusion but I'm not blocking the PR for this.
## Relevant issue(s) Resolves #2643 ## Description Don't setup http and p2p by default in test suite. This is a regression I introduced in #2641 (sorry for the bother) - we set p2p and http up later. As the new, unused, P2P and http servers werent cleaned up at the end of the test, this had quite a bad effect on test suite memory consumption. Later, I think the surrounding code that sets up p2p and http could probably be changed later to make better use of `node.New`, but IMO this is good enough for now (we have other things to do).
…work#2641) ## Relevant issue(s) Resolves sourcenetwork#2634 ## Description Tests node pkg constructor via integration test suite instead of bypassing it and directly creating `db` instances via the `db` package.
…work#2644) ## Relevant issue(s) Resolves sourcenetwork#2643 ## Description Don't setup http and p2p by default in test suite. This is a regression I introduced in sourcenetwork#2641 (sorry for the bother) - we set p2p and http up later. As the new, unused, P2P and http servers werent cleaned up at the end of the test, this had quite a bad effect on test suite memory consumption. Later, I think the surrounding code that sets up p2p and http could probably be changed later to make better use of `node.New`, but IMO this is good enough for now (we have other things to do).
Relevant issue(s)
Resolves #2634
Description
Tests node pkg constructor via integration test suite instead of bypassing it and directly creating
dbinstances via thedbpackage.