Skip to content

Add CLI tests#86

Merged
dzmitryhil merged 4 commits intomasterfrom
dzmitryhil/cli-tests
Jan 12, 2024
Merged

Add CLI tests#86
dzmitryhil merged 4 commits intomasterfrom
dzmitryhil/cli-tests

Conversation

@dzmitryhil
Copy link
Contributor

@dzmitryhil dzmitryhil commented Jan 10, 2024

Description

Add CLI tests.

Reviewers checklist:

  • Try to write more meaningful comments with clear actions to be taken.
  • Nit-picking should be unblocking. Focus on core issues.

Authors checklist

  • Provide a concise and meaningful description
  • Review the code yourself first, before making the PR.
  • Annotate your PR in places that require explanation.
  • Think and try to split the PR to smaller PR if it is big.

This change is Reviewable

@dzmitryhil dzmitryhil requested a review from a team as a code owner January 10, 2024 05:47
@dzmitryhil dzmitryhil requested review from miladz68, wojtek-coreum and ysv and removed request for a team January 10, 2024 05:47
Copy link
Collaborator

@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.

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


relayer/cmd/cli/cli.go line 65 at r1 (raw file):

// BridgeClient is bridge client used to interact with the chains and contract.
//
//nolint:interfacebloat

I tend to agree with this linter that this interface looks too fat
Maybe we should split it like this:

type BridgeClientXRPL interface {
  // All xrpl related methods go here
}

type BridgeClientCoreum interface {
  // All Coreum related methods go here
}

type BridgeClient interface {
  BridgeClientXRPL
  BridgeClientCoreum
  
  // other methods
}

relayer/cmd/cli/cli.go line 440 at r1 (raw file):

// RecoverTicketsCmd recovers 250 tickets in the bridge contract.
func RecoverTicketsCmd(bcp BridgeClientProvider) *cobra.Command {

why do we need to do it like this ? I mean to pass func to fetch client from cmd


relayer/cmd/cli/cli_test.go line 75 at r1 (raw file):

func TestRelayerKeyInfoCmd(t *testing.T) {
	unsealConfig()

I would try to avoid this bug unless it is absolutely necessary to have.

Maybe here we can just merge this 2 tests into 1 as exception ? Or we plan more tests where this hack is needed ?


relayer/cmd/cli/cli_test.go line 141 at r1 (raw file):

	cmd := cli.ContractConfigCmd(func(cmd *cobra.Command) (cli.BridgeClient, error) {
		return bridgeClientMock, nil
	})

I can see that this is repeated almost in each func. Can we move it somewhere to avoid code duplication ?

Option 1: move it to executeCmd so it looks like this: executeCmd(t, func, bridgeClientMock). But this will probably not work for keyring commands

option 2: define func similar to NewMockBridgeClient but it should return ClientProvider func as 2nd return param. And pass this func when calling each Cmd

Or maybe there is another option

Code quote:

	cmd := cli.ContractConfigCmd(func(cmd *cobra.Command) (cli.BridgeClient, error) {
		return bridgeClientMock, nil
	})

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.

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


relayer/cmd/cli/cli.go line 65 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I tend to agree with this linter that this interface looks too fat
Maybe we should split it like this:

type BridgeClientXRPL interface {
  // All xrpl related methods go here
}

type BridgeClientCoreum interface {
  // All Coreum related methods go here
}

type BridgeClient interface {
  BridgeClientXRPL
  BridgeClientCoreum
  
  // other methods
}

But that separation is similar to nolint IMO. We don't use the interface separately.


relayer/cmd/cli/cli.go line 440 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

why do we need to do it like this ? I mean to pass func to fetch client from cmd

Because we read the arguments from the CLI to init the runner and bridge client as well.


relayer/cmd/cli/cli_test.go line 75 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I would try to avoid this bug unless it is absolutely necessary to have.

Maybe here we can just merge this 2 tests into 1 as exception ? Or we plan more tests where this hack is needed ?

I tried to do it that way. But it doesn't work, since we set the SDK config for both CLI commands.


relayer/cmd/cli/cli_test.go line 141 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I can see that this is repeated almost in each func. Can we move it somewhere to avoid code duplication ?

Option 1: move it to executeCmd so it looks like this: executeCmd(t, func, bridgeClientMock). But this will probably not work for keyring commands

option 2: define func similar to NewMockBridgeClient but it should return ClientProvider func as 2nd return param. And pass this func when calling each Cmd

Or maybe there is another option

executeCmd is used with 2 mocks.
Create a helper function to remove the duplication.

Copy link
Collaborator

@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.

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

@dzmitryhil dzmitryhil merged commit a1dd510 into master Jan 12, 2024
@ysv ysv deleted the dzmitryhil/cli-tests branch June 4, 2025 18:48
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.

2 participants