Conversation
f456a98 to
7885a98
Compare
|
Hey @magik6k, I'm taking a look at this and I can follow what you did mechanically, but I don't understand why you did it? I have some questions; I apologize if they don't make sense.
Any other context you could provide quickly would be great. I get that I might just be the wrong reviewer but you mentioned this PR about two weeks ago and I wanted to get you some feedback. Thanks. |
core/coreapi/interface/tests/api.go
Outdated
| select { | ||
| case <-zeroRunning: | ||
| case <-time.After(time.Second): | ||
| t.Errorf("%d node(s) not closed", running) |
There was a problem hiding this comment.
nit: we're not really tracking nodes here right? More like constructed swarms.
ipfs-inactive/interface-js-ipfs-core#66, basically we don't want external implementations of coreapi to depend on go-ipfs repo
go-ipfs-http-api uses IPTB to spawn nodes and relies on contexts to kill those nodes.
Running all coreapi tests takes about 3-4s for the |
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
184c570 to
44fc750
Compare
eingenito
left a comment
There was a problem hiding this comment.
I don't fully understand the refactor going on, but I trust you do. I had no important comments, only one tiny thing.
| res, err := api.Block().Put(ctx, strings.NewReader(`Hello`)) | ||
| if err != nil { | ||
| t.Error(err) | ||
| t.Fatal(err) |
There was a problem hiding this comment.
What's the pattern for the replacement of Error with Fatal? Are we testing the first use of the CoreAPI generally?
There was a problem hiding this comment.
I think I understand - it's an existing inconsistency and you didn't want to change them wholesale.
There was a problem hiding this comment.
TBH I just started changing them here as I was working on running tests in go-ipfs-http-api and this got committed. I'll commit more of those fixes in another PR.
| func TestGenerateType(t *testing.T) { | ||
| ctx := context.Background() | ||
| func (tp *provider) TestGenerateType(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
These could go below skip?
There was a problem hiding this comment.
(yes but I didn't see that comment before I merged... oh well, it doesn't really matter much).
Needs to be done before interface extraction