Integration test for node selection (PR) #397

deyanzz · 2023-06-22T13:02:01Z

Description:

Related issue(s):

Fixes: #167

Notes for reviewer:

Checklist

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

rwalworth · 2023-10-04T15:18:33Z

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

…Test. Signed-off-by: Deyan Zhekov <deyan.zhekov@limechain.tech>

deyanzz · 2023-10-05T13:11:14Z

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 · 2023-10-05T14:24:48Z

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.

Signed-off-by: Deyan Zhekov <deyan.zhekov@limechain.tech>

rwalworth · 2023-10-11T14:25:20Z

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

deyanzz · 2023-10-11T14:38:09Z

@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.

Signed-off-by: Deyan Zhekov <deyan.zhekov@limechain.tech>

deyanzz · 2023-10-11T18:19:00Z

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

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

sdk/tests/integration/ClientIntegrationTest.cc

rwalworth · 2023-10-11T18:25:05Z

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));
+}


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.

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.

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.

Signed-off-by: Deyan Zhekov <deyan.zhekov@limechain.tech>

deyanzz linked an issue Jun 22, 2023 that may be closed by this pull request

Integration test for node selection #167

Closed

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 force-pushed the 00167-integration-test-for-node-selection branch from 3f792ea to bb5e2e6 Compare July 13, 2023 13:11

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 marked this pull request as ready for review September 8, 2023 13:05

deyanzz self-assigned this Sep 8, 2023

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 force-pushed the 00167-integration-test-for-node-selection branch 2 times, most recently from 505581b to de9ea0d Compare September 28, 2023 14:48

167: Add missing methods in Client. Add new test in ClientIntegration…

1abb56d

…Test. Signed-off-by: Deyan Zhekov <deyan.zhekov@limechain.tech>

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

397: Remove comments

894a45f

Signed-off-by: Deyan Zhekov <deyan.zhekov@limechain.tech>

167: Changes for test SetNetworkIsWorkingCorrectly.

191d650

Signed-off-by: Deyan Zhekov <deyan.zhekov@limechain.tech>

deyanzz requested a review from rwalworth October 11, 2023 17:39

deyanzz added the Improvement Code changes driven by non business requirements label Oct 11, 2023

rwalworth approved these changes Oct 11, 2023

View reviewed changes

deyanzz added 2 commits October 11, 2023 21:47

167: Resolve comments during PR.

b8259a7

Signed-off-by: Deyan Zhekov <deyan.zhekov@limechain.tech>

167: Fix for failing pipeline.

c1f5b98

Signed-off-by: Deyan Zhekov <deyan.zhekov@limechain.tech>

deyanzz merged commit 1faa00c into main Oct 12, 2023
2 checks passed

deyanzz deleted the 00167-integration-test-for-node-selection branch October 12, 2023 07:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration test for node selection (PR) #397

Integration test for node selection (PR) #397

deyanzz commented Jun 22, 2023 •

edited

rwalworth commented Oct 4, 2023

deyanzz commented Oct 5, 2023

rwalworth commented Oct 5, 2023

rwalworth commented Oct 11, 2023

deyanzz commented Oct 11, 2023

deyanzz commented Oct 11, 2023

rwalworth left a comment

rwalworth Oct 11, 2023

deyanzz Oct 12, 2023

rwalworth Oct 12, 2023

Integration test for node selection (PR) #397

Integration test for node selection (PR) #397

Conversation

deyanzz commented Jun 22, 2023 • edited

rwalworth commented Oct 4, 2023

deyanzz commented Oct 5, 2023

rwalworth commented Oct 5, 2023

rwalworth commented Oct 11, 2023

deyanzz commented Oct 11, 2023

deyanzz commented Oct 11, 2023

rwalworth left a comment

Choose a reason for hiding this comment

rwalworth Oct 11, 2023

Choose a reason for hiding this comment

deyanzz Oct 12, 2023

Choose a reason for hiding this comment

rwalworth Oct 12, 2023

Choose a reason for hiding this comment

deyanzz commented Jun 22, 2023 •

edited