RPC API Batch 1 - Acceptance Test - Improvement #1050

AlfredoG87 · 2023-04-12T00:57:58Z

Description:
changed the hardcoded value of "blocksBehindLatest" from 40 to 10, since is enough for the purpose of the test and gives higher chances that even if the tests and the hardware used to run the tests improves in the future, the test will continue to work.

Update:
Only if currentBlock is bigger than 10, we fetch past 10 blocks, otherwise we fetch everything since the block 0.

Related issue(s):

Fixes #1049

Notes for reviewer:

Checklist

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

…nce is enough for the purpose of the test and gives higher chances that even if the tests and the hardware used to run the tests improves in the future, the test will continue to work. Signed-off-by: Alfredo Gutierrez <[email protected]>

Signed-off-by: Alfredo Gutierrez <[email protected]>

codecov-commenter · 2023-04-12T01:02:58Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (a53d454) 74.59% compared to head (2fb437d) 74.59%.

❗ Current head 2fb437d differs from pull request most recent head c0f07f6. Consider uploading reports for the commit c0f07f6 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1050   +/-   ##
=======================================
  Coverage   74.59%   74.59%           
=======================================
  Files          31       31           
  Lines        2035     2035           
  Branches      387      387           
=======================================
  Hits         1518     1518           
  Misses        410      410           
  Partials      107      107

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Nana-EC

Single consideration to make check more robust

Nana-EC · 2023-04-12T03:57:04Z

packages/server/tests/acceptance/rpc_batch1.spec.ts

@@ -281,7 +281,8 @@ describe('@api-batch-1 RPC Server Acceptance Tests', function () {
                //for the purpose of the test, we are settings limit to 2, and fetching all.
                //setting mirror node limit to 2 for this test only
                process.env['MIRROR_NODE_LIMIT_PARAM'] = '2';
-                const blocksBehindLatest = Number(await relay.call('eth_blockNumber', [], requestId)) - 40;
+                // 10 is an arbitrary number and might be changed in the future for a smaller number in case test fails due to negative number in the fromBlock param
+                const blocksBehindLatest = Number(await relay.call('eth_blockNumber', [], requestId)) - 10;


Is it worth making sure the block count is always above this arbitrary number to avoid a negative number as you noted?

Suggested change

const blocksBehindLatest = Number(await relay.call('eth_blockNumber', [], requestId)) - 10;

const blockOffset = 10;

const latestBlockNum = await relay.call('eth_blockNumber', [], requestId))

const minBlockNum = min(latestBlockNum, blockOffset); // not sure if a further offset of 1 is needed

const blocksBehindLatest = minBlockNum - blockOffset;

I've changed it to check if CurrentBlock is bigger than 10, otherwise fetch from block 0 (the beginning).
also, the test needs at least 3 or more to pass.

…less than 10 Signed-off-by: Alfredo Gutierrez <[email protected]>

Signed-off-by: Alfredo Gutierrez <[email protected]>

sonarqubecloud · 2023-04-12T15:22:43Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

ebadiere

LGTM.

* changed the hardcoded value of "blocksBehindLatest" from 40 to 10, since is enough for the purpose of the test and gives higher chances that even if the tests and the hardware used to run the tests improves in the future, the test will continue to work. Signed-off-by: Alfredo Gutierrez <[email protected]> * adding comments to change Signed-off-by: Alfredo Gutierrez <[email protected]> * further improve test to fetch from beginning in case currentBlock is less than 10 Signed-off-by: Alfredo Gutierrez <[email protected]> * avoid double call of eth_blockNumber Signed-off-by: Alfredo Gutierrez <[email protected]> --------- Signed-off-by: Alfredo Gutierrez <[email protected]>

* changed the hardcoded value of "blocksBehindLatest" from 40 to 10, since is enough for the purpose of the test and gives higher chances that even if the tests and the hardware used to run the tests improves in the future, the test will continue to work. * adding comments to change * further improve test to fetch from beginning in case currentBlock is less than 10 * avoid double call of eth_blockNumber --------- Signed-off-by: Alfredo Gutierrez <[email protected]> Co-authored-by: Alfredo Gutierrez <[email protected]>

AlfredoG87 added 2 commits April 11, 2023 18:55

adding comments to change

c0f07f6

Signed-off-by: Alfredo Gutierrez <[email protected]>

AlfredoG87 self-assigned this Apr 12, 2023

AlfredoG87 requested review from georgi-l95, ebadiere, Nana-EC and lukelee-sl April 12, 2023 00:58

AlfredoG87 marked this pull request as ready for review April 12, 2023 01:08

Nana-EC added P2 process Build, test and deployment-process related tasks enhancement New feature or request labels Apr 12, 2023

Nana-EC added this to the 0.22.0 milestone Apr 12, 2023

Nana-EC reviewed Apr 12, 2023

View reviewed changes

AlfredoG87 added 2 commits April 12, 2023 09:16

further improve test to fetch from beginning in case currentBlock is …

9b257f9

…less than 10 Signed-off-by: Alfredo Gutierrez <[email protected]>

avoid double call of eth_blockNumber

a4b2733

Signed-off-by: Alfredo Gutierrez <[email protected]>

AlfredoG87 requested a review from Nana-EC April 12, 2023 15:55

Nana-EC approved these changes Apr 12, 2023

View reviewed changes

ebadiere reviewed Apr 13, 2023

View reviewed changes

AlfredoG87 merged commit 0fc591b into main Apr 13, 2023

AlfredoG87 deleted the 1049-improve-api-batch1-acceptancetest branch April 13, 2023 15:53

Nana-EC mentioned this pull request Apr 15, 2023

RPC API Batch 1 - Acceptance Test - Improvement (#1050) #1075

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC API Batch 1 - Acceptance Test - Improvement #1050

RPC API Batch 1 - Acceptance Test - Improvement #1050

AlfredoG87 commented Apr 12, 2023 •

edited

Loading

codecov-commenter commented Apr 12, 2023

Nana-EC left a comment

Nana-EC Apr 12, 2023

AlfredoG87 Apr 12, 2023 •

edited

Loading

sonarqubecloud bot commented Apr 12, 2023

ebadiere left a comment

-                const blocksBehindLatest = Number(await relay.call('eth_blockNumber', [], requestId)) - 10;
+                const blockOffset = 10;
+                const latestBlockNum = await relay.call('eth_blockNumber', [], requestId))
+                const minBlockNum = min(latestBlockNum, blockOffset); // not sure if a further offset of 1 is needed
+                const blocksBehindLatest = minBlockNum - blockOffset;

RPC API Batch 1 - Acceptance Test - Improvement #1050

RPC API Batch 1 - Acceptance Test - Improvement #1050

Conversation

AlfredoG87 commented Apr 12, 2023 • edited Loading

codecov-commenter commented Apr 12, 2023

Codecov Report

Nana-EC left a comment

Choose a reason for hiding this comment

Nana-EC Apr 12, 2023

Choose a reason for hiding this comment

AlfredoG87 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 12, 2023

ebadiere left a comment

Choose a reason for hiding this comment

AlfredoG87 commented Apr 12, 2023 •

edited

Loading

AlfredoG87 Apr 12, 2023 •

edited

Loading