[WIP]fraud/testing: Use header/mocks.Store instead of custom mockStore #1674
Conversation
This change modifies the test file in the fraud pkg to make use of mockStore in libs/header/mocks. Fixes: celestiaorg#1542 Signed-off-by: Richard Gregory <richardgrecoson@gmail.com>
richardgreg
requested review from
renaynay,
Wondertan,
vgonkivs and
distractedm1nd
as code owners
|
It was meant to be a straightforward modification, but a new change made it a little complicated, at least for me. There are now two different header packages:
|
|
@distractedm1nd do you understand the issue I explained in the comments? |
There was a problem hiding this comment.
Thanks for tackling this,@richardgreg, and sorry that it took us so much time to get here.
Also lint is broken + needs rebase
| store := &mockStore{ | ||
| headers: make(map[int64]*header.ExtendedHeader), | ||
| headHeight: 0, | ||
| Headers: make(map[int64]H), | ||
| HeadHeight: 0, | ||
| } | ||
|
|
||
| suite := headertest.NewTestSuite(t, numHeaders) | ||
|
|
||
| for i := 0; i < numHeaders; i++ { | ||
| header := suite.GenExtendedHeader() | ||
| store.headers[header.Height()] = header | ||
| store.Headers[header.Height()] = header | ||
|
|
||
| if header.Height() > store.headHeight { | ||
| store.headHeight = header.Height() | ||
| if header.Height() > store.HeadHeight { | ||
| store.HeadHeight = header.Height() | ||
| } | ||
| } |
There was a problem hiding this comment.
We should use mocks.NewStore instead. It already implements header gen, when testsuite is passed to it as Generator.
| } | ||
| type H libheader.Header | ||
|
|
||
| type mockStore mockstore.MockStore[H] |
There was a problem hiding this comment.
We don't need to define another type can use MockStore directly
There was a problem hiding this comment.
Then we wouldn't be able to define methods on them; at least, that's the error I get when I call headerMock.MockStore instead of the custom mockStore
p.s. My knowledge of GO isn't deep, so if I sound too naive, I can skip this problem and take on another one 🙂
There was a problem hiding this comment.
Hey @richardgreg no worries! If you want to define additional methods on MockStore, you are welcome to add them in libs/header/mocks/store.go if they are necessary/useful :) So no need to alias the type here. We'd like to keep MockStore and all its methods within the scope of the libs/header/mocks package anyway.
There was a problem hiding this comment.
Hey @richardgreg are you still working on this? If not, we can take it over no problem!
There was a problem hiding this comment.
Hi @renaynay, I kept running into dead ends when attempting to fix this. It's evident I need to improve my GO skills. Yeah, you can take it over. 🙂
|
@richardgreg, gentle ping |
|
Hi @Wondertan, still here. I got your feedback; I'll update you tomorrow :) |
|
@richardgreg, gentle nudge |
|
Hi @Wondertan, I added a response here 3 days ago. Can I close this so someone else can work on it? |
renaynay
added
area:fraud
kind:testing
|
Closing as it's now fixed by #2039 |
This change modifies the test file in the fraud pkg to make use of mockStore in libs/header/mocks.
Fixes: #1542
Signed-off-by: Richard Gregory richardgrecoson@gmail.com
Overview
Checklist