added init function to ibc integration tests package #574
Conversation
miladz68
requested review from
dzmitryhil,
vertex451,
ysv and
wojtek-coreum
and removed request for
a team
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)
integration-tests/ibc/init.go line 30 at r1 (raw file):
func parseIBCFlags() { flagSet := flag.NewFlagSet("ibc", flag.ExitOnError)
What is the purpose of that flag?
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @silverspase, @wojtek-coreum, and @ysv)
integration-tests/ibc/init.go line 30 at r1 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
What is the purpose of that flag?
to parse ibc specific flags
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @silverspase, @wojtek-coreum, and @ysv)
integration-tests/ibc/init.go line 30 at r1 (raw file):
Previously, miladz68 (milad) wrote…
to parse ibc specific flags
moved them back to integrationtest package, since they errored out here.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)
integration-tests/ibc/init.go line 30 at r1 (raw file):
Previously, miladz68 (milad) wrote…
moved them back to integrationtest package, since they errored out here.
Didn't get last comment, in what case they errors out?
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @silverspase, @wojtek-coreum, and @ysv)
integration-tests/ibc/init.go line 30 at r1 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Didn't get last comment, in what case they errors out?
if we start znet with integrationtest-module, then passing ibc info to test binary will error out.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)
a discussion (no related file):
I still encourage to consider lazy-loading. The less code in init the better
integration-tests/init.go line 50 at r2 (raw file):
var ( ctx context.Context ChainsHolder Chains
I liked "Chains" more
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)
integration-tests/init.go line 50 at r2 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
I liked "Chains" more
Me too
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)
a discussion (no related file):
Previously, wojtek-coreum (Wojtek) wrote…
I still encourage to consider lazy-loading. The less code in init the better
Done.
integration-tests/init.go line 50 at r2 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Me too
Me too.
but suggest names for the struct so the name will not conflict with var name. I am out of ideas.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)
a discussion (no related file):
Previously, miladz68 (milad) wrote…
Done.
Actually trying to this seems unnecesarily complicated. since the current solution works, it is good enough.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)
integration-tests/init.go line 50 at r2 (raw file):
Previously, miladz68 (milad) wrote…
Me too.
but suggest names for the struct so the name will not conflict with var name. I am out of ideas.
Decided to use NewCoreumTestingContext and NewChainsTestingContext for the initialisation.
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @silverspase, @wojtek-coreum, and @ysv)
integration-tests/init.go line 50 at r2 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Decided to use NewCoreumTestingContext and NewChainsTestingContext for the initialisation.
Done.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68, @silverspase, and @wojtek-coreum)
integration-tests/init.go line 153 at r3 (raw file):
// GetGaiaChain returns the configured gaia chain. func GetGaiaChain(t *testing.T) Chain {
you keep these methods in integration-tests not ibc on purpose, right ?
Because I thought maybe it makes sense to move it to ibc.
On the other hand we should still keep flags in integration-tests
integration-tests/ibc/asset_ft_test.go line 340 at r3 (raw file):
t.Parallel() ctx, coreumChain := integrationtests.NewCoreumTestingContext(t)
when we were on the call I proposed idea to keep the interface of NewCoreumTestingContext(t) and trigger lazy loading inside it.
So NewCoreumTestingContext should still return chains as slice and call GetGaiaChain & GetOsmosisChain inside. The main idea was to keep same funcs & their interfaces.
Benefit of your approach is that you init what you actually need I think.
But if you prefer current version I'm ok to discuss.
There was a problem hiding this comment.
Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)
integration-tests/init.go line 153 at r3 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
you keep these methods in integration-tests not ibc on purpose, right ?
Because I thought maybe it makes sense to move it to ibc.
On the other hand we should still keep flags in integration-tests
My understanding of the discussion on the call was that it makes sense to keep the flag definition and variable initialization in the same place, and not to distribute the logic. this was the argument against using separate init functions.
integration-tests/ibc/asset_ft_test.go line 340 at r3 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
when we were on the call I proposed idea to keep the interface of
NewCoreumTestingContext(t)and trigger lazy loading inside it.So NewCoreumTestingContext should still return chains as slice and call GetGaiaChain & GetOsmosisChain inside. The main idea was to keep same funcs & their interfaces.
Benefit of your approach is that you init what you actually need I think.
But if you prefer current version I'm ok to discuss.
since we are using lazy loading and defining new functions anyways, the struct is not really helpful anyways, so I decided to remove it.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @wojtek-coreum)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @silverspase, and @wojtek-coreum)
integration-tests/init.go line 153 at r4 (raw file):
// GetGaiaChain returns the configured gaia chain. func GetGaiaChain(t *testing.T) Chain {
But we agreed to do the initialisation on integrationtests.NewCoreumTestingContext(t)
@ysv right?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @silverspase, and @wojtek-coreum)
integration-tests/init.go line 153 at r4 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
But we agreed to do the initialisation on
integrationtests.NewCoreumTestingContext(t)
@ysv right?
Same for the integrationtests.NewChains. In that case you won't even need to update tests.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)
integration-tests/init.go line 153 at r4 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Same for the
integrationtests.NewChains. In that case you won't even need to update tests.
that would defeat the purpose of this PR, the purpose is to initialize the first time it is needed. if it is initialized in NewCoreumTestingContext, then it will error out in case IBC components are not started but znet.
There was a problem hiding this comment.
Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)
integration-tests/init.go line 50 at r2 (raw file):
Previously, miladz68 (milad) wrote…
Done.
Pushed new changes. review again
integration-tests/init.go line 153 at r4 (raw file):
Previously, miladz68 (milad) wrote…
that would defeat the purpose of this PR, the purpose is to initialize the first time it is needed. if it is initialized in NewCoreumTestingContext, then it will error out in case IBC components are not started but znet.
agreed, new changes are pushed now
There was a problem hiding this comment.
Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @silverspase, and @wojtek-coreum)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vertex451 and @wojtek-coreum)
There was a problem hiding this comment.
Reviewed 1 of 5 files at r3, 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vertex451 and @wojtek-coreum)
There was a problem hiding this comment.
Dismissed @wojtek-coreum from a discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @vertex451)
This change is