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

Move ETH tests to the new testnet #1828

Merged
merged 11 commits into from
May 22, 2023
Merged

Move ETH tests to the new testnet #1828

merged 11 commits into from
May 22, 2023

Conversation

caglaryucekaya
Copy link

This PR:

  • Changes the URLs and contract addresses used for ETH tests
  • Reduces the amount of ETH and ERC20 tokens exchanged due to limited test coin funds, and changes the logic of the test if necessary, e.g. using fixed addresses instead of funding one-time randomly generated addresses
  • Fixes existing bugs for some tests

@shamardy
Copy link
Collaborator

@caglaryucekaya the below tests pass locally but fail in CI, can you please fix them?

mm2_tests::mm2_tests_inner::test_eth_swap_contract_addr_negotiation_same_fallback
mm2_tests::mm2_tests_inner::test_eth_swap_negotiation_fails_maker_no_fallback
mm2_tests::mm2_tests_inner::test_my_orders_after_matched
mm2_tests::mm2_tests_inner::test_update_maker_order_after_matched
mm2_tests::mm2_tests_inner::test_withdraw_and_send

@caglaryucekaya
Copy link
Author

@caglaryucekaya the below tests pass locally but fail in CI, can you please fix them?

mm2_tests::mm2_tests_inner::test_eth_swap_contract_addr_negotiation_same_fallback
mm2_tests::mm2_tests_inner::test_eth_swap_negotiation_fails_maker_no_fallback
mm2_tests::mm2_tests_inner::test_my_orders_after_matched
mm2_tests::mm2_tests_inner::test_update_maker_order_after_matched
mm2_tests::mm2_tests_inner::test_withdraw_and_send

The errors in CI are caused by a connection problem to electrum nodes, which also happen on the dev branch so it's not relevant to the changes in this PR.

@shamardy
Copy link
Collaborator

The errors in CI are caused by a connection problem to electrum nodes, which also happen on the dev branch so it's not relevant to the changes in this PR.

I reran the tests in CI and they still fail, they also don't fail in other branches/PRs. It's not related to electrum nodes connections, e.g. for test_eth_swap_contract_addr_negotiation_same_fallback the error is the following
https://github.com/KomodoPlatform/atomicDEX-API/actions/runs/5002237645/jobs/8970513468?pr=1828#step:4:1542

thread 'mm2_tests::mm2_tests_inner::test_eth_swap_contract_addr_negotiation_same_fallback' panicked at '!buy: {"error":"rpc:211] dispatcher_legacy:141] lp_ordermatch:3754] taker_swap:2285] eth:4672] Transport error: Got invalid response: http://195.201.137.5:8545/: data did not match any variant of untagged enum Response"}', /home/runner/work/atomicDEX-API/atomicDEX-API/mm2src/mm2_test_helpers/src/for_tests.rs:2756:9

Which is related to the new ethereum testnet node, this is the same error that I got before when trying to use the new testnet for trade_test_electrum_and_eth_coins.

@caglaryucekaya
Copy link
Author

caglaryucekaya commented May 18, 2023

I reran the tests in CI and they still fail, they also don't fail in other branches/PRs. It's not related to electrum nodes connections, e.g. for test_eth_swap_contract_addr_negotiation_same_fallback the error is the following https://github.com/KomodoPlatform/atomicDEX-API/actions/runs/5002237645/jobs/8970513468?pr=1828#step:4:1542

You're right, I was having the electrum node problem locally, the problem on CI had a different reason. get_passphrase!(".env.seed", "BOB_PASSPHRASE") and get_passphrase!(".env.client", "ALICE_PASSPHRASE") macros used in the tests are giving different results locally and on CI, so we're using different addresses on CI which weren't funded, causing the tests to fail. You were getting the same error last week because you were using a token contract address that wasn't funded. The problem is fixed after I also funded the CI addresses, but is there a reason why we're using different addresses? For windows tests, we're also using a different address pair.

@shamardy
Copy link
Collaborator

but is there a reason why we're using different addresses? For windows tests, we're also using a different address pair.

I don't know why different bob/alice passphrases are used in different CI machines, that's how it was in the old CI workflow and we wanted to keep it the same. If I had to guess why this decision was made in the past, it would be to prevent race conditions when running the CI jobs in parallel.

@caglaryucekaya
Copy link
Author

but is there a reason why we're using different addresses? For windows tests, we're also using a different address pair.

I don't know why different bob/alice passphrases are used in different CI machines, that's how it was in the old CI workflow and we wanted to keep it the same. If I had to guess why this decision was made in the past, it would be to prevent race conditions when running the CI jobs in parallel.

Yeah that could be the reason, I'm having similar issues on watchtower tests when I use the same addresses and the tests are running in parallel. It can stay this way then, managing the balance of three pairs of addresses isn't a big problem.

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this much needed PR! We finally have a healthy/green CI pipeline!
I only have a few minor comments.

mm2src/coins/eth/eth_tests.rs Show resolved Hide resolved
mm2src/mm2_test_helpers/src/for_tests.rs Show resolved Hide resolved
mm2src/mm2_test_helpers/src/for_tests.rs Show resolved Hide resolved
mm2src/coins/eth/eth_tests.rs Show resolved Hide resolved
mm2src/coins/eth/eth_tests.rs Show resolved Hide resolved
shamardy
shamardy previously approved these changes May 22, 2023
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

LGTM!
To merge this PR, please add a changelog entry and add a todo for this #1828 (comment)

@shamardy shamardy merged commit 4f97d69 into dev May 22, 2023
@shamardy shamardy deleted the eth-new-testnet branch May 22, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants