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

Tests: libwallet_api_tests fixing scripts #8264

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mj-xmr
Copy link
Contributor

@mj-xmr mj-xmr commented Apr 18, 2022

As a first step towards fixing the libwallet_api_tests, I updated the relevant scripts. I based my work on @MartyHav's commits, but extended them further. Also updated documentation.

@mj-xmr mj-xmr force-pushed the tests-libwallet_api-scripts-fix branch from 08d9505 to 35e18c5 Compare April 21, 2022 05:55
@jeffro256
Copy link
Contributor

I'm guessing that there's some historical reason for the mismatch, but shouldn't the default testnet port be 28081, not 38081?. This could possible be confusing for someone running these tests

@jeffro256
Copy link
Contributor

Also, what do you mean by "fixing" in this instance? Besides that, I really like how much cleaner the scripts are.

@mj-xmr
Copy link
Contributor Author

mj-xmr commented Apr 27, 2022

Also, what do you mean by "fixing" in this instance? Besides that, I really like how much cleaner the scripts are.

The way they were written, using them lead to segfault of the tests.

@mj-xmr
Copy link
Contributor Author

mj-xmr commented Apr 27, 2022

I'm guessing that there's some historical reason for the mismatch, but shouldn't the default testnet port be 28081, not 38081?. This could possible be confusing for someone running these tests

I'll have a closer look and answer later.

@mj-xmr
Copy link
Contributor Author

mj-xmr commented Apr 29, 2022

I'm guessing that there's some historical reason for the mismatch, but shouldn't the default testnet port be 28081, not 38081?. This could possible be confusing for someone running these tests

Here are the test's defaults:

https://github.com/monero-project/monero/blob/master/tests/libwallet_api_tests/main.cpp#L90-L91

std::string TESTNET_DAEMON_ADDRESS = "localhost:38081";
std::string MAINNET_DAEMON_ADDRESS = "localhost:18081";

If the documentation / scripts are meant to describe the current state, then I think it's fine. Should there be changes to these ports, then they'd have to be widely communicated first and documented in other HOWTOs across the space.

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.

2 participants