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

Add e2e plugin test for chainfee #273

Merged
merged 19 commits into from
Oct 30, 2024
Merged

Add e2e plugin test for chainfee #273

merged 19 commits into from
Oct 30, 2024

Conversation

0xAustinWang
Copy link
Collaborator

@0xAustinWang 0xAustinWang commented Oct 29, 2024

@0xAustinWang 0xAustinWang changed the title initial test, not working on mock Add e2e plugin test for chainfee Oct 29, 2024
@0xAustinWang 0xAustinWang marked this pull request as ready for review October 29, 2024 12:00
@0xAustinWang 0xAustinWang requested a review from a team as a code owner October 29, 2024 12:00
commit/plugin_e2e_test.go Outdated Show resolved Hide resolved
commit/plugin_e2e_test.go Outdated Show resolved Hide resolved
commit/plugin_e2e_test.go Outdated Show resolved Hide resolved
Comment on lines +509 to +511
destChain: newNativePrice,
sourceChain1: newNativePrice,
sourceChain2: newNativePrice,
Copy link
Contributor

Choose a reason for hiding this comment

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

The assertions would be a lot easier to read if the parameter was these prices rather than the prices and all the expectations

n := setupNode(params, oracleIDs[i])
nodes[i] = n.node

prepareCcipReaderMock(
Copy link
Contributor

@winder winder Oct 29, 2024

Choose a reason for hiding this comment

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

I guess you could add the fee components as arguments to this function

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 fee components are on a different reader, but i added a function for those

commit/plugin_e2e_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

This looks about right, but I'm not familiar enough with the chainfee logic to tell if you missed anything.

Left a couple suggestions that would help clarify the tests. It would also be helpful to leave a few extra comments for each test that explain why the inputs result in the expected outcome.

Copy link

Metric e2e/chainfeeTest main
Coverage 72.1% 72.0%

@0xAustinWang 0xAustinWang merged commit 528ecac into main Oct 30, 2024
4 checks passed
@mateusz-sekara mateusz-sekara deleted the e2e/chainfeeTest branch November 8, 2024 09:19
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.

4 participants