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

CCIP-4165 additional e2e integration tests #15245

Conversation

jhweintraub
Copy link
Collaborator

Additional End-to-End integration tests for CCIP involving token transfers paying in different feeTokens

Copy link
Contributor

@makramkd makramkd left a comment

Choose a reason for hiding this comment

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

Awesome! Are there any commonalities from each test case that can be pulled out into funcs? See e.g messagingTestCase in ccip_messaging_test.go

integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved
integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved
integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved
integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved
integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved
integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved
integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Nov 21, 2024

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@jhweintraub jhweintraub requested a review from a team as a code owner November 25, 2024 19:14
@jhweintraub jhweintraub requested a review from a team as a code owner November 25, 2024 19:14
@jhweintraub jhweintraub force-pushed the CCIP-4165-e-2-e-tests-implement-pricing-for-tokens-transfers-tests branch from d99db51 to ad9aea9 Compare November 25, 2024 19:35
integration-tests/smoke/ccip/ccip_test.go Outdated Show resolved Hide resolved
Comment on lines 583 to 584
// The balance should be 4 since we've already sent 2 tokens over this lane direction in the
// first part of the test, so balance should already be two
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to avoid sequential testing as this will cost the execution time. I would advice to see how we can make these test runs in parallel.
Few options are like:

  1. Using multiple wallet address
  2. Creating it as individual test which has its own setup and runner to run in parallel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

running in parallel with multiple wallets makes the most sense as the original ticket specified under the same e2e test.

delete(expectedSeqNum, sourceDestPair)
})

t.Run("Send Token Pay with native remote chain -> home", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the difference in this test is only about fee token, then we can pass that as an argument and simplify the test.

}},
}

t.Run("Send message Pay with Link token home chain -> remote", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As our intention is to test fee token, I think we can avoid testing this is in different lanes. We can test it in the same lane with different token by that we can simplify the test more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The original ticket specified that each test scenario should flip the lane direction

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel it is not needed. @makramkd @mateusz-sekara What do you guys think about this?

delete(expectedSeqNum, sourceDestPair)
})

t.Run("Send message pay with wrapped native home chain -> remote", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can do something like this:

  1. Setup one lane say home chain -> remote
  2. Create test input struct with fee token type (link, native and wnative) and message type (Token, Data and Token+Data)
  3. Loop the test and try to run in parallel using multiple wallet address.

WDYT?

use the fee token paid from the event rather than the call before the
request is made
@makramkd makramkd requested review from a team as code owners November 28, 2024 16:33
makramkd
makramkd previously approved these changes Nov 28, 2024
@makramkd makramkd enabled auto-merge November 28, 2024 18:15
@makramkd makramkd added this pull request to the merge queue Nov 28, 2024
Merged via the queue into develop with commit 86f83f4 Nov 28, 2024
173 of 174 checks passed
@makramkd makramkd deleted the CCIP-4165-e-2-e-tests-implement-pricing-for-tokens-transfers-tests branch November 28, 2024 18:42
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