Skip to content

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

Closed
clemahieu wants to merge 24 commits intonanocurrency:developfrom
clemahieu:bootstrap_test_add_node
Closed

Convert unit test make_shared<nano::node> to system.add_node.#4368
clemahieu wants to merge 24 commits intonanocurrency:developfrom
clemahieu:bootstrap_test_add_node

Conversation

@clemahieu
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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