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

ci: multichain-testing with go-relayer #10182

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Oct 2, 2024

closes: #10179

Description

Ensures compatibility with go-relayer by adding workflow dispatch jobs to multichain-e2e for Orchestration API (config.go-relayer.yaml) and FastUSDC (config.fusdc.go-relayer.yaml). Our existing tests using hermes. Scope is mainly for the Orchestration API, but x/vibc is also covered to an extent.

Towards this goal:

  • enable starship faucet for agoric in all configs (required for go-relayer)
  • update channel-close tests to use go-relayer when RELAYER_TYPE env var is present

Security Considerations

n/a, test code

Scaling Considerations

Not currently part of integration tests that run on merges to master.

Documentation Considerations

Added a section in the README.md with instructions for running the tests with go-relayer.

Testing Considerations

Runs existing test suite against go-relayer, extending our test coverage across to the two main cosmos-sdk relayer clients. This should help identify any relayer-specific issues or discrepancies. Please note: these tests will not run on every PR and must be dispatched manually.

Upgrade Considerations

n/a

@0xpatrickdev 0xpatrickdev added the force:integration Force integration tests to run on PR label Oct 2, 2024
Copy link

cloudflare-workers-and-pages bot commented Oct 2, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3f0038f
Status: ✅  Deploy successful!
Preview URL: https://929eb5d1.agoric-sdk.pages.dev
Branch Preview URL: https://10179-go-relayer.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev force-pushed the 10179-go-relayer branch 9 times, most recently from cd1daba to 2d38529 Compare October 3, 2024 18:45
@0xpatrickdev 0xpatrickdev marked this pull request as ready for review October 3, 2024 19:38
@0xpatrickdev 0xpatrickdev requested a review from a team as a code owner October 3, 2024 19:38
@0xpatrickdev 0xpatrickdev added force:integration Force integration tests to run on PR and removed force:integration Force integration tests to run on PR labels Oct 3, 2024
@0xpatrickdev 0xpatrickdev requested review from turadg and LuqiPan October 3, 2024 22:34
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Good stuff!

.github/workflows/multichain-e2e-template.yml Outdated Show resolved Hide resolved
.github/workflows/multichain-e2e-template.yml Outdated Show resolved Hide resolved
.github/workflows/multichain-e2e.yml Show resolved Hide resolved
multichain-testing/package.json Outdated Show resolved Hide resolved
@0xpatrickdev
Copy link
Member Author

0xpatrickdev commented Oct 4, 2024

Hesitant to merge until we see this go through CI multiple times successfully.

I was under the impression each run is fully containerized, but am seeing weird behavior which leads me to believe these are stepping on each other's toes. Both jobs read from and write to the agoric-sdk/multichain-testing filesystem and access the same ports over the network.

Any folks with more gh-action-fu have more insights?

Edit: Bad hunch - everything is isolated in GitHub action runs, even without service containers. I was mainly thrown off by an opaque error in a starships client dependency. It also seems like the "Setup Starship Infrastructure" step can complete successfully without establishing connections and channels. For now, we can check in go-relayer tests as dispatch (on-demand) jobs to avoid flakes.

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

I'll approve when the work-arounds for upstream issues are gone, or there's a conspicuous plan for addressing them.

.github/workflows/multichain-e2e-template.yml Outdated Show resolved Hide resolved
.github/workflows/multichain-e2e-template.yml Outdated Show resolved Hide resolved
multichain-testing/README.md Outdated Show resolved Hide resolved
@0xpatrickdev 0xpatrickdev force-pushed the 10179-go-relayer branch 3 times, most recently from f0827ce to 11015a0 Compare October 16, 2024 19:09
@0xpatrickdev 0xpatrickdev marked this pull request as draft October 16, 2024 19:16
@0xpatrickdev 0xpatrickdev force-pushed the 10179-go-relayer branch 4 times, most recently from fa4b501 to 4414f4c Compare October 17, 2024 00:26
@0xpatrickdev 0xpatrickdev force-pushed the 10179-go-relayer branch 12 times, most recently from 82709be to a0c73e0 Compare October 18, 2024 14:09
- to match ChainName conventions used in `@cosmos/chain-registry`
- adds dispatch jobs to test orchestration-api and fusdc test suites against go-relayer
@0xpatrickdev 0xpatrickdev marked this pull request as ready for review January 14, 2025 00:20
Copy link
Member Author

@0xpatrickdev 0xpatrickdev left a comment

Choose a reason for hiding this comment

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

It looks like GH has an outage, but this is R4R. I amended this comment with some details. I think the current approach renders a majority of the current feedback no longer relevant - lmk if that's not the case or you need me to jog your memory on anything @turadg.

@0xpatrickdev 0xpatrickdev requested a review from turadg January 14, 2025 00:21
name: Multichain E2E (Orchestration API - Hermes)
if: |
github.event_name == 'workflow_call' ||
(github.event_name == 'workflow_dispatch' && inputs.test_type == 'orchestration-api-hermes')
Copy link
Member

Choose a reason for hiding this comment

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

please update the job names to match the test_type selector.

test_command: test:fast-usdc
test_command: yarn test:fast-usdc

# Below jobs run on dispatch only
Copy link
Member

Choose a reason for hiding this comment

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

this kind of comment might be missed when someone changes whether it's true.

please comment on each job's if with when it runs

@@ -23,6 +23,8 @@ import type { CurrentWalletRecord } from '@agoric/smart-wallet/src/smartWallet.j
import type { QueryBalanceResponseSDKType } from '@agoric/cosmic-proto/cosmos/bank/v1beta1/query.js';
import type { USDCProposalShapes } from '@agoric/fast-usdc/src/pool-share-math.js';

const RELAYER_TYPE = process.env.RELAYER_TYPE;
Copy link
Member

Choose a reason for hiding this comment

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

to make clear the name is unchanged,

Suggested change
const RELAYER_TYPE = process.env.RELAYER_TYPE;
const { RELAYER_TYPE } = process.env;

t.log(e); // hermes relayer throws, but go-relayer does not
}
t.log('Sleeping 10 seconds for potential channel closure to propagate');
await sleep(10000);
Copy link
Member

Choose a reason for hiding this comment

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

how was 10s determined? please make it more legible:

Suggested change
await sleep(10000);
await sleep(10 * 1000);

@@ -41,3 +41,8 @@ export const MAKE_ACCOUNT_AND_QUERY_BALANCE_TIMEOUT: RetryOptions = {
retryIntervalMs: 5000,
maxRetries: 24,
};

export const TWO_MINUTES: RetryOptions = {
Copy link
Member

Choose a reason for hiding this comment

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

please give this a semantic name. It's baking in more assumptions than two minutes

@@ -70,7 +70,11 @@ const makeKeyring = async (

export const commonSetup = async (
t: ExecutionContext,
{ config = '../config.yaml' } = {},
{
env = process.env,
Copy link
Member

Choose a reason for hiding this comment

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

why propagate the whole env? if someone wants to override relayerType they don't have to mess with env at all

t.deepEqual(balance, { denom: bondDenom, amount: '10000000000' });
const walletScenario = test.macro({
title: (_, chainName: string) =>
`create a wallet and get tokens on ${chainName}`,
Copy link
Member

Choose a reason for hiding this comment

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

worth it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multichain Testing - support go-relayer
2 participants