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

Convert unit test make_shared<nano::node> to system.add_node. #4368

Closed

Conversation

clemahieu
Copy link
Contributor

When changing the lifetime of io_context it ocurred that a lot of unit tests constructed nodes manually via make_shored.

This change converts many usages to system.add_node. Some usages like the networking tests are not upgraded as they're testing specific operations on how nodes connect which system::add_node assumes.

@clemahieu clemahieu requested a review from dsiganos January 18, 2024 01:33
dsiganos
dsiganos previously approved these changes Jan 18, 2024
@dsiganos dsiganos self-requested a review January 18, 2024 03:35
@dsiganos
Copy link
Contributor

I think some of these tests manually create a node so that there are no automatic network connections between the nodes.

@dsiganos
Copy link
Contributor

For example, the test case bootstrap_processor.lazy_hash_pruning intentionally creates the nano_node instance without using add_node so that there are no connections between the 2 nodes and then manually does a tcp_establish to connect the two nodes when the test wishes to connect them.

the test expects node1 to be created without network connections
@dsiganos
Copy link
Contributor

@clemahieu I made some fixes to the tests and committed, please check that you agree with what I am doing, before I carry on and do the rest

@clemahieu
Copy link
Contributor Author

Yes these fixes look good. I thought I had avoided conversions in tests that needed a starting disconnected node, for instance the network.* tests, but I missed these bootstrap ones.

@dsiganos dsiganos force-pushed the bootstrap_test_add_node branch from 6e09c5f to 753360f Compare January 19, 2024 09:53
@dsiganos
Copy link
Contributor

The principle idea behind this PR is invalid. The idea is to modify the bootstrap and rep_remove tests to system.add_node() instead of manually making their own node. But they manually make their own node because they want a disconnected node. I have made a lot of improvements to the bootstrap unit tests, including comments to stop this mistake from happening again (it has happened before too), but I will commit them as a new PR.

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