Conversation
chatton
force-pushed
the
cian/fix-integration-tests
branch
from
ca165e5 to
f5554dc
Compare
internal/comet/comet_conn.go
Outdated
| ) | ||
|
|
||
| // NewCometGRPCConn creates a new gRPC client connection with retry interceptors and connectivity checks. | ||
| func NewCometGRPCConn(endpoint string) (*grpc.ClientConn, error) { |
There was a problem hiding this comment.
would maybe suggest going with the naming convention of "core" rather than "comet" for this and the package naming. Or just pkg naming core.NewCometGRPCConn
nodebuilder/tests/api_test.go
Outdated
| sw := swamp.NewSwamp(t, swamp.WithBlockTime(time.Second)) | ||
| // start a bridge node | ||
| bridge := sw.NewBridgeNode() | ||
| bridge := sw.NewBridgeNode(fx.Replace(sw.Fetcher())) |
There was a problem hiding this comment.
any idea what a swamp is and why this needs to replace a "swamp fetcher"? 😂
There was a problem hiding this comment.
yeah that was the main gotcha in this PR, the Fetcher is a BlockFetcher, which maybe it should be renamed to, which uses an underlying comet/core grpc connection.
The problem is that unless it is replaced, a new instance is returned which does not use the correct gRPC connection and things blow up.
There may be a nicer way to do this, but we can refactor the fx.Replace if we have a cleaner way!
nodebuilder/tests/prune_test.go
Outdated
| // spin up 3 pruning FNs, connect | ||
| // spin up 1 LN that syncs historic blobs | ||
| func TestArchivalBlobSync(t *testing.T) { | ||
| t.Skip("FIXME(chatton): error with fx.Replace") |
There was a problem hiding this comment.
wanna maybe add another todo in a comment so they can all be grepped by the same string
| switch tp { | ||
| case node.Bridge: | ||
| host, port, err := net.SplitHostPort(s.ClientContext.GRPCClient.Target()) | ||
| host, port, err := net.SplitHostPort(s.ClientContext.GRPCClient.Target()) // TODO(chatton) should this be comet address or app address? |
There was a problem hiding this comment.
I'm going to assume its comet/core.
what is it configured to right now? if tests are passing maybe its configured correctly?
There was a problem hiding this comment.
right now it is configured to the app gRPC endpoint, since tests are passing I also assume it is fine, but I want this to show up in the final diff to ensure that is correct.
| ca.stakingCli = stakingtypes.NewQueryClient(ca.appConn) | ||
| ca.feeGrantCli = feegrant.NewQueryClient(ca.appConn) |
There was a problem hiding this comment.
so these don't work with the coreConn?
There was a problem hiding this comment.
the tests pass with both, but actually I don't see any usages of the calling functions that use these underlying clis. I will add another TODO here to verify this with the node team.
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
|
In a follow up, I think we can use named fx providers to specify 2 different grpc connections. And remove the replace statements. |
2 participants
This PR updates the integration tests so they all pass
There was just a single test that is skipped and will be addressed in a follow up.
The main gotcha I ran into was with regards to
fx.Replace(sw.BlockFetcher(), this ensures that the sameBlockFetcherinstance is returned from thefxdependency injection library.This didn't matter before because there was a single grpc connection used to communicate with the app and with comet, now they are separated, so the issue was that a different Fetcher instance was being returned but was wired up pointing at the app grpc endpoint.
Added TODOs for open questions.
Linting will be fixed later, was having issues running it locally.