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

PoC: CLI parsing for itest tapd harness queries #1311

Merged
merged 9 commits into from
Jan 22, 2025
Merged

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Jan 20, 2025

This PR is a preliminary step toward reproducing the commands detailed in external-group-key.md within our integration tests.


This PR introduces a new package, cmd/commands, to encapsulate command-line parsing functionality, enabling direct interaction with itest tapd harnesses via CLI functionality. The goal is to integrate command-line parsing into our integration tests to ensure CLI functionality is thoroughly tested.

Summary of Changes

  1. Package Refactoring: Migrated command-line parsing logic to cmd/commands, laying the groundwork for modular testing.
  2. CLI Integration: Introduced ExecTapCLI to execute commands directly against itest tapd harnesses, facilitating CLI-driven tests.
  3. Targeted RPC: Wrapped command actions to target specific RPC clients, streamlining harness interaction.
  4. CLI testing for testGetInfo: Updated the getinfo integration test to validate results via CLI commands, demonstrating proof-of-concept functionality.

Export a new function `NewApp` to handle app construction.

This change simplifies the transition when `commands.go` is moved
to a new package by reducing the number of exported symbols,
resulting in a cleaner refactor.
Preemptively address lint errors in preparation for refactoring.
The code will be moved to a new file, which would trigger linter
complaints. Fixing these issues now ensures a clean copy and
allows the refactor commit to pass linting.
This change prepares for a future commit that will consolidate
all command-line functionality into a new package.
Export `Fatal` to ensure it remains accessible in `tapcli`
after migrating it to a new package.
@ffranr ffranr requested a review from guggero January 20, 2025 17:33
@ffranr ffranr self-assigned this Jan 20, 2025
@ffranr ffranr removed the request for review from guggero January 20, 2025 17:34
@ffranr
Copy link
Contributor Author

ffranr commented Jan 20, 2025

I think one nice side effect of this is that the command action function (getInfo in the PoC) is simpler because the client is initiated in the action wrapper.

@coveralls
Copy link

coveralls commented Jan 20, 2025

Pull Request Test Coverage Report for Build 12893807284

Details

  • 0 of 461 (0.0%) changed or added relevant lines in 4 files are covered.
  • 35 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.09%) to 40.673%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/tapcli/main.go 0 3 0.0%
itest/tapd_harness.go 0 36 0.0%
cmd/commands/conn.go 0 203 0.0%
cmd/commands/commands.go 0 219 0.0%
Files with Coverage Reduction New Missed Lines %
cmd/tapcli/main.go 1 0.0%
commitment/tap.go 1 83.64%
asset/asset.go 2 77.13%
version.go 3 11.24%
tapdb/universe.go 4 80.91%
asset/mock.go 4 92.13%
tapchannel/aux_leaf_signer.go 5 43.08%
universe/interface.go 15 50.65%
Totals Coverage Status
Change from base Build 12893024407: -0.09%
Covered Lines: 26645
Relevant Lines: 65510

💛 - Coveralls

@ffranr ffranr force-pushed the tapcliutil-package branch 2 times, most recently from 780d1ba to f264abd Compare January 20, 2025 19:11
@guggero
Copy link
Member

guggero commented Jan 21, 2025

Just a drive-by comment: Would be nice if we could structure things the same as in lnd: Have a cmd/commands package instead of tapcliutil.
IMO that has several advantages:

  1. It's easily visible from the import path that the package contains CLI functionality so that doesn't have to be in the package name itself
  2. The import name will be just commands by default
  3. Anything CLI related will be grouped under cli/

Relocate command-line parsing functionality to a new package
called `commands`. This change will eventually enable
integration tests to call into the parsing logic directly.
@ffranr ffranr force-pushed the tapcliutil-package branch from f264abd to bcb4cd5 Compare January 21, 2025 11:40
@guggero guggero self-requested a review January 21, 2025 12:15
@ffranr ffranr marked this pull request as draft January 21, 2025 13:03
@ffranr ffranr force-pushed the tapcliutil-package branch from bcb4cd5 to 7c06a51 Compare January 21, 2025 14:50
@ffranr ffranr marked this pull request as ready for review January 21, 2025 14:54
@ffranr ffranr requested a review from GeorgeTsagk January 21, 2025 14:54
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice! Super useful to get more test coverage 💯

cmd/commands/commands.go Show resolved Hide resolved
@@ -68,6 +68,21 @@ func getClient(ctx *cli.Context) (taprpc.TaprootAssetsClient, func()) {
return taprpc.NewTaprootAssetsClient(conn), cleanUp
}

// RpcBundle is an interface that bundles all the client interfaces
// that are used by the tapcli.
type RpcBundle interface {
Copy link
Member

Choose a reason for hiding this comment

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

We could move TapdClient here instead, so we only have one place to update:

type TapdClient interface {

Copy link
Contributor Author

@ffranr ffranr Jan 21, 2025

Choose a reason for hiding this comment

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

Good point! I hadn't realised before now that we only had that sort of interface in itest. There's a far bit of code to move around and simplify on that front. I'll spin it into a new PR when I've got a bit more time.

Let me know if you think I should work differently etc.

Copy link
Member

Choose a reason for hiding this comment

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

We could move TapdClient here instead, so we only have one place to update:

that seems to be the RPC client interface, how could it reside in the commands package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could move TapdClient here instead, so we only have one place to update:

that seems to be the RPC client interface, how could it reside in the commands package?

hmm will require some thought

Introduce functionality to wrap command actions, enabling them
to target a specific RPC client (e.g., the `itest` tapd harness)
directly. This eliminates the need to parse command-line arguments
to determine the target tapd instance.
Apply the new RPC client targeting functionality to the `getinfo`
command, allowing it to directly target (call into) a specific RPC
client.
Introduce the `ExecTapCLI` function to execute arbitrary CLI commands
against a specific tapd integration test instance. This enables
running CLI commands on itest harnesses and retrieving their
responses.
Update the existing `getinfo` integration test to ensure the
same results can be reproduced using the new command-line
functionality.
@ffranr ffranr force-pushed the tapcliutil-package branch from 7c06a51 to 2a3ca21 Compare January 21, 2025 18:50
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -118,4 +118,14 @@ func testGetInfo(t *harnessTest) {
// Ensure network field is set correctly.
expectedNetwork := t.tapd.cfg.NetParams.Name
require.Equal(t.t, expectedNetwork, resp.Network)

// Attempt to get the info using the CLI.
respGeneric, err := ExecTapCLI(ctxt, t.tapd, "getinfo")
Copy link
Member

Choose a reason for hiding this comment

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

👍 could also (as part of the PoC) assert a path where the command fails and returns an err, non-blocking

would also be nice to add some basic coverage on all commands in a future PR

@@ -68,6 +68,21 @@ func getClient(ctx *cli.Context) (taprpc.TaprootAssetsClient, func()) {
return taprpc.NewTaprootAssetsClient(conn), cleanUp
}

// RpcBundle is an interface that bundles all the client interfaces
// that are used by the tapcli.
type RpcBundle interface {
Copy link
Member

Choose a reason for hiding this comment

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

We could move TapdClient here instead, so we only have one place to update:

that seems to be the RPC client interface, how could it reside in the commands package?

@ffranr ffranr added this pull request to the merge queue Jan 22, 2025
Merged via the queue into main with commit ac2961a Jan 22, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants