js/data: ModuleV2 migration #2243
Conversation
codebien
force-pushed
the
data-modulev2
branch
2 times, most recently
from
a815e60
to
d7cc535
Compare
na--
requested review from
oleiade and
yorugac
and removed request for
inancgumus
codebien
force-pushed
the
data-modulev2
branch
from
d7cc535
to
b55d77d
Compare
| if !ok { | ||
| return rt, fmt.Errorf("not a Data module instance") | ||
| } |
There was a problem hiding this comment.
Perhaps, not even a request change, but a curiosity question: @codebien, why didn't you use require.True(t, ok) here?
There was a problem hiding this comment.
assert|require with concurrency could generate a race so I tend to avoid it for helper functions if it's possible (if the func return error). Otherwise, I would prefer a classic require.True as you suggested.
There was a problem hiding this comment.
I think what you are talking about @codebien is that t.FailNow and co. are supposed to be called only from the goroutine that is running the test, not by anyone that is spawned by it.
We do ... not do that quite a lot it just so happens that in most cases the failure never happens 😄
I think it's perfectly fine to leave the error instead of using require.True and co., but also in this case I would expect it's perfectly fine to use it as well
There was a problem hiding this comment.
that in most cases the failure never happens
Sure, but in case of changes for refactoring, you could get the race.
but also in this case I would expect it's perfectly fine to use it as well
Yep, but I don't know how the helper could be used in the next test we will write. For this reason, I think it makes sense just for helpers shared across multiple tests. Ofc, in a single test I would prefer to use directly the assert function.
Migrated the js/data API to the new modules.Module interface (aka ModuleV2).
codebien
force-pushed
the
data-modulev2
branch
from
b55d77d
to
00f42b0
Compare
Migrated
k6/datato themodules.Module(akamodules.ModuleV2) interface.