swarm/network/stream: refactor bootstrapping of tests #1176
Conversation
|
Note that there were number of places where we were not calling |
Good catch @nonsense. I thought that network shutdown will do that, but actually it will just call Stop method on Service (in this case Registry). Registry closing is required for closing intervals database. |
| n := ctx.Config.Node() | ||
|
|
||
| store, datadir, err := createTestLocalStorageForID(n.ID(), addr) | ||
| if *useMockStore { |
There was a problem hiding this comment.
@janos I am pretty sure this doesn't work, but haven't tried it just yet. Not sure how we cast a mock store to *storage.LocalStore further down, but this is how it is in the current code as well.
There was a problem hiding this comment.
We have found that mock store in stream tests were broken after some rewrite. I am not sure if they were fixed. Maybe @holisticode have more details, but I will have a look also.
There was a problem hiding this comment.
But it is important that mock store is not casted to LocalStore, there were numerous discussion on this, but injected down to ldbstore.
There was a problem hiding this comment.
At least now we have to fix it at one place, and not at multiple :)
There was a problem hiding this comment.
MockStore is working correctly here, but naming is confusing. createMockStore does not create mock store, it creates new localstore with injected mockstore. It is similar to createTestLocalStorageForID, so maybe to rename this function to createTestLocalStoreWithMockStore, or newLocalStoreWithMock, and to move it from syncer_test.go to common_test.go. BTW, the description of this flag is completely wrong, it is false by default.
nonsense
force-pushed
the
sync-monitor-anton
branch
from
5a23519 to
98de1bd
Compare
|
Rebased on |
| n := ctx.Config.Node() | ||
|
|
||
| store, datadir, err := createTestLocalStorageForID(n.ID(), addr) | ||
| if *useMockStore { |
There was a problem hiding this comment.
MockStore is working correctly here, but naming is confusing. createMockStore does not create mock store, it creates new localstore with injected mockstore. It is similar to createTestLocalStorageForID, so maybe to rename this function to createTestLocalStoreWithMockStore, or newLocalStoreWithMock, and to move it from syncer_test.go to common_test.go. BTW, the description of this flag is completely wrong, it is false by default.
|
|
||
| fileStore := storage.NewFileStore(netStore, storage.NewFileStoreParams()) | ||
| bucket.Store(bucketKeyFileStore, fileStore) | ||
| r := NewRegistry(addr.ID(), delivery, netStore, state.NewInmemoryStore(), &RegistryOptions{ |
There was a problem hiding this comment.
What if we'd just add a RegistryOptions object to the newNetStoreAndDeliveryWithRequestFunc, which then also would contain this NewRegistry code? I know it starts to look ugly with so many params...but it could further simplify simulation creation
There was a problem hiding this comment.
@holisticode we override the registry in some of the tests, this is why I didn't do it - its not just the RegistryOptions that change.
Personally I think even the current constructors are a bit too complicated, but this is because of the high coupling between the interfaces...
The current solution is far from ideal, I just hope it bring slightly better defaults.
|
merged as ethereum/go-ethereum#18975 |
After reviewing @holisticode 's PR ethereum/go-ethereum#18972 and discussing my opinion on the test it was adding, I looked into refactoring the service bootstrap that is replicated over 10 simulation tests.
I came up with 3 constructors that pretty much abstract the functionality we need:
These reduce the copy-pasted boilerplate that we have in the simulation tests.
I didn't get as far as reviewing the constructor for
NewRegistry, but maybe there are things we could simplify there as well.Let me know what you think.