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.

Sign up for GitHub

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

Jump to bottom

Integration test for node selection (PR) #397

Merged
merged 5 commits into from
Oct 12, 2023
Merged

Integration test for node selection (PR) #397

merged 5 commits into from
Oct 12, 2023

Conversation

deyanzz
Copy link
Contributor

@deyanzz deyanzz commented Jun 22, 2023 •
edited
Loading

Description:

Related issue(s):

Fixes: #167

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@deyanzz deyanzz linked an issue Jun 22, 2023 that may be closed by this pull request
Integration test for node selection #167
Closed
@deyanzz deyanzz force-pushed the 00167-integration-test-for-node-selection branch 3 times, most recently from 6250586 to 3f792ea Compare June 29, 2023 15:06
@deyanzz deyanzz force-pushed the 00167-integration-test-for-node-selection branch from 3f792ea to bb5e2e6 Compare July 13, 2023 13:11
@deyanzz deyanzz force-pushed the 00167-integration-test-for-node-selection branch 3 times, most recently from c158843 to 577a0f2 Compare September 5, 2023 16:04
@deyanzz deyanzz marked this pull request as ready for review September 8, 2023 13:05
@deyanzz deyanzz self-assigned this Sep 8, 2023
@deyanzz deyanzz force-pushed the 00167-integration-test-for-node-selection branch 2 times, most recently from 78c7532 to 6557329 Compare September 25, 2023 13:31
@deyanzz deyanzz force-pushed the 00167-integration-test-for-node-selection branch 2 times, most recently from 505581b to de9ea0d Compare September 28, 2023 14:48
@rwalworth
Copy link
Contributor

Hey @deyanzz what's going on with this PR? It was opened a while ago but I'm not sure if it's complete/ready to review

@deyanzz deyanzz force-pushed the 00167-integration-test-for-node-selection branch from de9ea0d to 1abb56d Compare October 5, 2023 10:27
@deyanzz
Copy link
Contributor Author

deyanzz commented Oct 5, 2023

Hey @deyanzz what's going on with this PR? It was opened a while ago but I'm not sure if it's complete/ready to review

Yes, I was hanging around with this PR for too long. I was wondering how exactly to implement the algorithm for node selection. I was searching for a similar tests in the Java SDK, but I found only one test to change the network in ClientTest.java.
Unfortunately, I wasn't able to find any kind of integration test for node selection in the JavaScript SDK.

Definitely, it's better to start a discussion in this PR to think about the best algorithm for node selection.

@rwalworth
Copy link
Contributor

Hey @deyanzz what's going on with this PR? It was opened a while ago but I'm not sure if it's complete/ready to review

Yes, I was hanging around with this PR for too long. I was wondering how exactly to implement the algorithm for node selection. I was searching for a similar tests in the Java SDK, but I found only one test to change the network in ClientTest.java. Unfortunately, I wasn't able to find any kind of integration test for node selection in the JavaScript SDK.

Definitely, it's better to start a discussion in this PR to think about the best algorithm for node selection.

The algorithm is already implemented in Network::getNodeAccountIdsForExecute(), the tests just need to make sure that correct nodes are picked. I'm thinking for now it may be best to wait to implement these tests after we are able to simulate node connectivity behavior (i.e. we're able to control multiple nodes in a local setup and are able set them to be healthy/unhealthy). Having this will allow us to test the full breadth of node selection. I'd recommend closing this PR for now and putting this task on the back burner until we have that functionality, which I believe will be a part of the FST tool that the hedera-local-node team is working on at the moment.

@deyanzz
397: Remove comments
894a45f 
Signed-off-by: Deyan Zhekov <[email protected]>
@rwalworth
Copy link
Contributor

@deyanzz are you good if I close this for now?

@deyanzz
Copy link
Contributor Author

deyanzz commented Oct 11, 2023

@deyanzz are you good if I close this for now?

Sorry for my late response. I'm preparing a commit, where will introduce a new test similar to setNetworkWorks from the Java SDK.

Also, I've done some minor refactoring for ClientIntegrationTest.

@deyanzz deyanzz requested a review from rwalworth October 11, 2023 17:39
@deyanzz deyanzz added the Improvement Code changes driven by non business requirements label Oct 11, 2023
@deyanzz
Copy link
Contributor Author

deyanzz commented Oct 11, 2023

With commit 191d650 I moved all of the unit tests for min & max backoff time into ClientTest.
Also, I'm suggesting to add a new integration test SetNetworkIsWorkingCorrectly in ClientIntegrationTest.

Created new issue #522 to track missing integration test for node selection.

rwalworth
rwalworth approved these changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@rwalworth rwalworth left a comment

Choose a reason for hiding this comment

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

LGTM, but there should be some follow-up on these once the multi-node test setup is done.

sdk/tests/integration/ClientIntegrationTest.cc Outdated Show resolved Hide resolved
sdk/tests/integration/ClientIntegrationTest.cc Outdated Show resolved Hide resolved
sdk/tests/integration/ClientIntegrationTest.cc
ASSERT_NO_THROW(accountBalance_6 = AccountBalanceQuery().setAccountId(accountId_6).execute(client));
ASSERT_NO_THROW(accountBalance_7 = AccountBalanceQuery().setAccountId(accountId_7).execute(client));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now but I think we should really try to enforce going forward that the integration tests do not communicate with previewnet/testnet/mainnet nodes, hence my previous comments about waiting for the multi-node setup to be complete. A user should be able to test locally with that set-up if they so desire, and seeing as how this test contains URLs for testnet consensus nodes, these tests would not pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree with you - using hardcoded URLs in the integration tests is a bad practice.
Integration tests should communicate only with the local node and this approach should be used in all of the Hedera's SDKs.
For now I will suggest to add a new JSON config files containing all of the consensus nodes for mainnet / previewnet / test. I will track this change in #525.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary as we should not be communicating with testnet nodes to begin with. We would add this JSON file ultimately to remove it when we get the multi-node setup integrated. I think we should leave it for now and then go through this again when we get the multi-node setup working.

@deyanzz deyanzz merged commit 1faa00c into main Oct 12, 2023
2 checks passed
@deyanzz deyanzz deleted the 00167-integration-test-for-node-selection branch October 12, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwalworth rwalworth rwalworth approved these changes

Assignees

@deyanzz deyanzz

Labels
Improvement Code changes driven by non business requirements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integration test for node selection
2 participants
@deyanzz @rwalworth