Rework rlpx-simulator.spec.ts tests / Fix devp2p/genesis/wallet Coverage Reporting #3760
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
| it('RLPX: remove node', async () => { | ||
| const { rlpxs, peer } = util.initTwoPeerRLPXSetup(undefined, undefined, undefined, 40504) | ||
| rlpxs[0] | ||
| ['_dpt']!.addPeer(peer) |
There was a problem hiding this comment.
The reason I moved the addPeer call out of initTwoPeerRLPXSetup is to be able to chain the disconnect here before checking for peer:removed events in the awaited promise that follows.
| assert.equal(rlpxs[0]._getOpenSlots(), 9, 'should have maxPeers - 1 open slots left') | ||
| await util.delay(500) | ||
| util.destroyRLPXs(rlpxs) | ||
| const { rlpxs, peer } = util.initTwoPeerRLPXSetup(undefined, undefined, undefined, basePort + 1) |
There was a problem hiding this comment.
I believe basePort + 1 is correct here since the following assert is checking if the peer's port is connecting as expected.
| throw new Error(`Peering failed: ${e}: ${e.stack}`) | ||
| }) | ||
| await new Promise((resolve) => { | ||
| rlpxs[0].events.once('peer:removed', (_, reason: any) => { | ||
| assert.equal( |
There was a problem hiding this comment.
This assert is failing, expecting a CLIENT_QUITTING but getting a TIMEOUT.
|
Can you add a readiness label (+ drop a comment on that)? |
…hereumjs-monorepo into rlpx-simulator-tests-fixes
…hereumjs-monorepo into rlpx-simulator-tests-fixes
|
Could you give a summary of what you finally did here and how things played out? |
…to rlpx-simulator-tests-fixes
| const basePort = 60661 | ||
| const rlpxs = util.getTestRLPXs(3, 1, basePort) | ||
| const peer = { address: util.localhost, udpPort: basePort + 1, tcpPort: basePort + 1 } | ||
| rlpxs[0]['_dpt']!.addPeer(peer) | ||
| try { |
There was a problem hiding this comment.
I reworked this test case to be a lot more similar to how the test was originally written to preserve the test coverage it gives for queuing/refilling connections while also updating it to run asynchronously.
| } | ||
| }) | ||
| }, 30000) | ||
| it('RLPX: remove node', async () => { |
There was a problem hiding this comment.
I both swapped around the final two test cases and increased the timeout of one of them since they have some interference happening with one another: They both pass in isolation and when ran in this new relative order, but fail with the old order they were in. Not sure if the cleanup is experiencing issues.
|
Thanks for the insights! 🙏 And on the more general level, so this re-activates test running in general, is this correct? Can we "prove" this is working by coverage already? If not we should get the coverage running again as a first step maybe and then test here with the fix, at least that's what I would suggest if I get everything "right" here and over on the coverage front. 🙂 |
Yes, this should activate tests that were previously not running fully to show lines covered increase in codecov relative to the base commit. |
…to rlpx-simulator-tests-fixes
…hereumjs-monorepo into rlpx-simulator-tests-fixes
|
I switched UPDATE: I wasn't able to get coverage reports generated for the |
holgerd77
changed the title
This change looks into the
rlpx-simulator.spec.tstests in thedevp2ppackage and attempts to understand and fix the test cases and if any expected bugs are found, they will be discussed here and fixed in followup PRs. Also see #3755.