Merge e2e tests to sim tests #4759
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
reviewed
Nov 11, 2022
dapplion
reviewed
Nov 11, 2022
| }); | ||
| await unknownBlockSync.jobs.el.start(); | ||
| await unknownBlockSync.jobs.cl.start(); | ||
| const head = await env.nodes[0].cl.api.beacon.getBlock("head"); |
There was a problem hiding this comment.
You should also assert that this node syncs to the head, you can bundle with the other sync tests. Just do this setup before hand
nazarhussain
force-pushed
the
nh/4275-e2e-to-sim
branch
from
7d9d497
to
38d6c44
Compare
dapplion
reviewed
Nov 15, 2022
dapplion
reviewed
Nov 15, 2022
| }); | ||
| await unknownBlockSync.jobs.el.start(); | ||
| await unknownBlockSync.jobs.cl.start(); | ||
| const head = await env.nodes[0].cl.api.beacon.getBlockV2("head"); |
There was a problem hiding this comment.
You must assert that unknownBlockSync syncs up to the block slot of head variable here.
dapplion
suggested changes
Nov 15, 2022
| }); | ||
| } | ||
| } | ||
| await waitForNodeSync(unknownBlockSync.nodePair, env.options.controller.signal); |
There was a problem hiding this comment.
This condition is not enough. You are just checking that at some point sync status it not syncing. You must assert that the head of this node is exactly head. Since this is not normal usage I can't do any guarantees of what will the syncing status do. Please do not use syncing status in unknown root sync test. For the rest of usage of waitForNodeSync you must assert that the node declares itself as synced + syncs past a specific head too.
There was a problem hiding this comment.
I've reviewed the logs on the unknown block sync node completes syncing at
Eph 6/6 0.986[unknown-block-sync-node-cl-lodestar/chain] �[36mverbose�[39m: New chain head headSlot=51, headRoot=0x74e92383d9084442240ee46fd1aaaeb3fed1bfd504b3764c48c700bc9e92e122, delaySec=12.986000061035156
Eph 6/6 0.986[unknown-block-sync-node-cl-lodestar/chain] �[36mverbose�[39m: Block processed slot=51, root=0x74e92383d9084442240ee46fd1aaaeb3fed1bfd504b3764c48c700bc9e92e122, delaySec=12.986000061035156
Eph 6/6 1.258[unknown-block-sync-node-cl-lodestar/rest] �[34mdebug�[39m: Req req-g 127.0.0.1 getSyncingStatus
Eph 6/6 1.259[unknown-block-sync-node-cl-lodestar/rest] �[34mdebug�[39m: Res req-g getSyncingStatus - 200
Eph 6/6 2.001[unknown-block-sync-node-cl-lodestar] �[32minfo�[39m: Synced - slot: 54 (skipped 3) - head: 51 0x74e9…e122 - execution: valid(0xc9a2…a9bb) -
The initial error is triggered at
Eph 6/4 0.208[unknown-block-sync-node-cl-lodestar/chain] �[31merror�[39m: Block error slot=51 code=BLOCK_ERROR_PARENT_UNKNOWN, parentRoot=0x62bfece4644b0c9901e8298eda2c926800991418308b09bb15c7b9e7326ef8f9
Which is the second to last block that's processed
Eph 6/6 0.971[unknown-block-sync-node-cl-lodestar/chain] �[36mverbose�[39m: New chain head headSlot=50, headRoot=0x62bfece4644b0c9901e8298eda2c926800991418308b09bb15c7b9e7326ef8f9, delaySec=16.97099995613098
Then the node stalls.
So yes, do not use syncing status here. The node returns not syncing just because it so happens to be close to the head, but that's a "coincidence".
There was a problem hiding this comment.
I did it as of the comment on the isSynced mentioned. In that controlled environment, don't think node can sync to any other head as we have assertions that heads on all nodes must be same.
Set to true if the node is syncing, false if the node is synced
Will extend the logic to track the head during the sync assertion.
dapplion
approved these changes
Nov 16, 2022
dapplion
pushed a commit
that referenced
this pull request
Nov 18, 2022
* Remove already covered tests * Move some instance functions to pure functions to be reuseable * Add uknonw block parent assertions * Update the wait time * Update block error message * Disable range sync for the uknown block sync node * Update the failing unit tests * Use getBlockV2 instead of getBlock * Fix linter warnings * Remove unknownBlockSync test covered in sim tests * Move few e2e tests to sim tests * Update the sim run command * Update docker runner * Fix child process stopping issue * Remove unnecessary e2e tests * Fix unit tests for new beacon options * Update a unit test * Remove an accidental committed file * Add sync check for the unkown sync node * Add simulation repoter interface * Update tracker event handling * Update repoter * Add logging for debugging * Update logic for head checking
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
To save time on CI jobs.
Description
Migrate
e2etests tosim-tests.Partially Resolves: #4275
Steps to test or reproduce
Run sim tests.