Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve integration tests timeouts to work with fast block time. #579

Merged
merged 4 commits into from
Jul 21, 2023

Conversation

dzmitryhil
Copy link
Contributor

@dzmitryhil dzmitryhil commented Jul 20, 2023

Description

The current integration tests' timeout is set with the expectation that the coreum block time and IBC chains block time are slower than 1 sec. So when we run them against the zent with faster block time, for example, 0.5sec, they pass with the time more than they can. Hence to improve it we can update the retry timeouts we use.


This change is Reviewable

@dzmitryhil dzmitryhil requested a review from a team as a code owner July 20, 2023 12:06
@dzmitryhil dzmitryhil requested review from vertex451, miladz68, ysv and wojtek-coreum and removed request for a team July 20, 2023 12:06
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering current values for retrials should we add a validation to crust to limit block time >0.5s ?

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @wojtek-coreum)


integration-tests/chain_ibc.go line 125 at r1 (raw file):

	retryCtx, retryCancel := context.WithTimeout(ctx, 30*time.Second)
	defer retryCancel()
	err := retry.Do(retryCtx, 100*time.Millisecond, func() error {

question:
Why do we have 500ms in some places & 100ms in other ?

what is the difference ?


integration-tests/ibc/transfer_test.go line 138 at r1 (raw file):

	osmosisChain := chains.Osmosis

	gaiaToCoreumChannelID := osmosisChain.AwaitForIBCChannelID(ctx, t, ibctransfertypes.PortID, coreumChain.ChainSettings.ChainID)

all further vars should also be renamed accordingly:
gaiaToCoreumChannelID -> osmosisToCoreumChannelID

etc

miladz68
miladz68 previously approved these changes Jul 20, 2023
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @silverspase, and @wojtek-coreum)

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually - not. The tests work well with any block time.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)


integration-tests/chain_ibc.go line 125 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

question:
Why do we have 500ms in some places & 100ms in other ?

what is the difference ?

100ms for the operation which we expect to be executed with low delay, e.g. tx submission. And 500ms for operations we expect to be executed with high delay, e.g. channels creation.


integration-tests/ibc/transfer_test.go line 138 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

all further vars should also be renamed accordingly:
gaiaToCoreumChannelID -> osmosisToCoreumChannelID

etc

Make sense. Updated.

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but for IBC tests it should be 1s+, no ?

image.png

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @wojtek-coreum)

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @wojtek-coreum)

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes for IBC it will be one sec.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @wojtek-coreum)

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @wojtek-coreum)

@dzmitryhil dzmitryhil merged commit 24be33d into master Jul 21, 2023
@dzmitryhil dzmitryhil deleted the dzmitryhil/fix-send-commision-test-namin branch July 21, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants