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

Rework rlpx-simulator.spec.ts tests / Fix devp2p/genesis/wallet Coverage Reporting #3760

Merged
merged 19 commits into from
Oct 31, 2024
Merged

Rework rlpx-simulator.spec.ts tests / Fix devp2p/genesis/wallet Coverage Reporting #3760

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
58523f9
Rework rlpx-simulator.spec.ts tests
scorbajio Oct 21, 2024
29fa66c
Fix twoPeerMsgExchange3
scorbajio Oct 21, 2024
e23680b
Merge branch 'master' into rlpx-simulator-tests-fixes
scorbajio Oct 22, 2024
055f677
Fix test
scorbajio Oct 28, 2024
7ba4cae
Merge branch 'rlpx-simulator-tests-fixes' of github.com:ethereumjs/et…
scorbajio Oct 29, 2024
c4a04b5
Merge branch 'master' into rlpx-simulator-tests-fixes
scorbajio Oct 29, 2024
414be15
Fix type errors
scorbajio Oct 29, 2024
3add80e
Merge branch 'rlpx-simulator-tests-fixes' of github.com:ethereumjs/et…
scorbajio Oct 29, 2024
79ea020
Restore tests to be more similar to previous format
scorbajio Oct 29, 2024
83422fb
Increase test timeout
scorbajio Oct 29, 2024
e21afb5
Merge branch 'master' of github.com:ethereumjs/ethereumjs-monorepo in…
scorbajio Oct 29, 2024
8cffe14
Merge branch 'master' into rlpx-simulator-tests-fixes
scorbajio Oct 29, 2024
c105b39
Merge branch 'master' into rlpx-simulator-tests-fixes
holgerd77 Oct 30, 2024
774bd6d
Merge branch 'master' of github.com:ethereumjs/ethereumjs-monorepo in…
scorbajio Oct 30, 2024
492c062
Fix devp2p coverage reporting
scorbajio Oct 30, 2024
ed6a457
Merge branch 'rlpx-simulator-tests-fixes' of github.com:ethereumjs/et…
scorbajio Oct 30, 2024
6980be5
Fix genesis codecov reporting
scorbajio Oct 30, 2024
98201ce
Update wallet codecov reporting command
scorbajio Oct 30, 2024
74da3dc
Merge branch 'master' into rlpx-simulator-tests-fixes
holgerd77 Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 72 additions & 66 deletions packages/devp2p/test/integration/rlpx-simulator.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,94 +6,100 @@
import * as util from './util.js'

describe('RLPx simulator tests', () => {
it('RLPX: add working node', () => {
it('RLPX: add working node', async () => {
const basePort = 40404
const rlpxs = util.initTwoPeerRLPXSetup(undefined, undefined, undefined, basePort)

rlpxs[0].events.on('peer:added', async (peer) => {
assert.equal(
peer['_port'],
basePort + 1,
'should have added peer on peer:added after successful handshake',
)
assert.equal(rlpxs[0].getPeers().length, 1, 'peer list length should be 1')
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)
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 believe basePort + 1 is correct here since the following assert is checking if the peer's port is connecting as expected.

rlpxs[0]['_dpt']!.addPeer(peer).catch(() => {
throw new Error('Peering failed')
})
await new Promise((resolve) => {
rlpxs[0].events.on('peer:added', async (peer) => {
assert.equal(
peer['_port'],
basePort + 1,
'should have added peer on peer:added after successful handshake',
)
assert.equal(rlpxs[0].getPeers().length, 1, 'peer list length should be 1')
assert.equal(rlpxs[0]._getOpenSlots(), 9, 'should have maxPeers - 1 open slots left')
await util.delay(500)
util.destroyRLPXs(rlpxs)
resolve(undefined)
})
})
})

it('RLPX: ban node with missing tcp port', () => {
const rlpxs = util.initTwoPeerRLPXSetup(undefined, undefined, undefined, 40444)

rlpxs[0].events.on('peer:added', async () => {
const peer = {
id: hexToBytes('0xabcd'),
address: '127.0.0.1',
udpPort: 30308,
tcpPort: null,
}
assert.notOk(
rlpxs[0]['_dpt']!['_banlist'].has(peer),
'should not be in ban list before bad peer discovered',
)
rlpxs[0]['_dpt']!.events.emit('peer:new', peer)
assert.ok(
rlpxs[0]['_dpt']!['_banlist'].has(peer),
'should be in ban list after bad peer discovered',
)
await util.delay(500)
util.destroyRLPXs(rlpxs)
it('RLPX: ban node with missing tcp port', async () => {
const { rlpxs, peer } = util.initTwoPeerRLPXSetup(undefined, undefined, undefined, 40444)
rlpxs[0]['_dpt']!.addPeer(peer).catch(() => {
throw new Error('Peering failed')
})
await new Promise((resolve) => {
rlpxs[0].events.on('peer:added', async () => {
const peer = {
id: hexToBytes('0xabcd'),
address: '127.0.0.1',
udpPort: 30308,
tcpPort: null,
}
assert.notOk(
rlpxs[0]['_dpt']!['_banlist'].has(peer),
'should not be in ban list before bad peer discovered',
)
rlpxs[0]['_dpt']!.events.emit('peer:new', peer)
assert.ok(
rlpxs[0]['_dpt']!['_banlist'].has(peer),
'should be in ban list after bad peer discovered',
)
await util.delay(500)
util.destroyRLPXs(rlpxs)
resolve(undefined)
})
})
})

it('RLPX: remove node', () => {
const rlpxs = util.initTwoPeerRLPXSetup(undefined, undefined, undefined, 40504)

try {
rlpxs[0].events.once('peer:added', (peer) => {
rlpxs[0].disconnect(peer['_remoteId'])
it('RLPX: remove node', async () => {
Copy link
Contributor Author

@scorbajio scorbajio Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

const { rlpxs, peer } = util.initTwoPeerRLPXSetup(undefined, undefined, undefined, 40504)
rlpxs[0]
['_dpt']!.addPeer(peer)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

.then((peer1) => {
rlpxs[0].disconnect(peer1['id'])
})
rlpxs[0].events.once('peer:removed', async (_, reason: any) => {
.catch((e) => {
throw new Error(`Peering failed: ${e}: ${e.stack}`)
})
await new Promise((resolve) => {
rlpxs[0].events.once('peer:removed', (_, reason: any) => {
assert.equal(

Check failure on line 70 in packages/devp2p/test/integration/rlpx-simulator.spec.ts

View workflow job for this annotation

GitHub Actions / devp2p / test-devp2p

Unhandled error 

AssertionError: should close with CLIENT_QUITTING disconnect reason: expected 11 to equal 8

- Expected
+ Received

- 8
+ 11

 ❯ EventEmitter.<anonymous> test/integration/rlpx-simulator.spec.ts:70:16
 ❯ EventEmitter.emit ../../node_modules/eventemitter3/index.js:183:35
 ❯ EventEmitter.<anonymous> src/rlpx/rlpx.ts:298:21
 ❯ EventEmitter.emit ../../node_modules/eventemitter3/index.js:182:35
 ❯ Peer._onSocketClose src/rlpx/peer.ts:661:38
 ❯ Object.onceWrapper node:events:634:26
 ❯ Socket.emit node:events:531:35
 ❯ TCP.<anonymous> node:net:339:12

This error originated in "test/integration/rlpx-simulator.spec.ts" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
The latest test that might've caused the error is "RLPX: test peer queue / refill connections". It might mean one of the following:
- The error was thrown, while Vitest was running this test.
- If the error occurred after the test had been completed, this was the last documented test before it was thrown.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This assert is failing, expecting a CLIENT_QUITTING but getting a TIMEOUT.

reason,
DISCONNECT_REASON.CLIENT_QUITTING,
'should close with CLIENT_QUITTING disconnect reason',
)
assert.equal(rlpxs[0]._getOpenSlots(), 10, 'should have maxPeers open slots left')
await util.delay(500)
util.destroyRLPXs(rlpxs)
})
} catch (err) {
assert.fail(`An unexpected error occurred: ${err}`)
}
resolve(undefined)
})
})

it('RLPX: test peer queue / refill connections', () => {
it('RLPX: test peer queue / refill connections', async () => {
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 {
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 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.

await new Promise((resolve) => {
rlpxs[0].events.on('peer:added', async (peer) => {
if ((peer['_socket'] as any)._peername.port === basePort + 1) {
assert.equal(rlpxs[0]['_peersQueue'].length, 0, 'peers queue should contain no peers')
const peer2 = {
address: util.localhost,
udpPort: basePort + 2,
tcpPort: basePort + 2,
}
rlpxs[0]['_dpt']!.addPeer(peer2)
await util.delay(500)
assert.equal(rlpxs[0]['_peersQueue'].length, 1, 'peers queue should contain one peer')
assert.equal(peer._socket._peername.port, basePort + 1)
assert.equal(rlpxs[0]['_peersQueue'].length, 0, 'peers queue should contain no peers')
const peer2 = {
address: util.localhost,
udpPort: basePort + 2,
tcpPort: basePort + 2,
}
if ((peer['_socket'] as any)._peername.port === basePort + 2) {
rlpxs[0]['_dpt']!.addPeer(peer2).then((peer) => {
assert.equal(rlpxs[0]['_peersQueue'].length, 1, 'peers queue should contain one peer')
assert.equal(peer.tcpPort, basePort + 2)
assert.equal(rlpxs[0]['_peersQueue'].length, 0, 'peers queue should contain no peers')

Check failure on line 98 in packages/devp2p/test/integration/rlpx-simulator.spec.ts

View workflow job for this annotation

GitHub Actions / devp2p / test-devp2p

Unhandled error 

AssertionError: peers queue should contain no peers: expected 1 to equal +0

- Expected
+ Received

- 0
+ 1

 ❯ test/integration/rlpx-simulator.spec.ts:98:18

This error originated in "test/integration/rlpx-simulator.spec.ts" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
The latest test that might've caused the error is "RLPX: test peer queue / refill connections". It might mean one of the following:
- The error was thrown, while Vitest was running this test.
- If the error occurred after the test had been completed, this was the last documented test before it was thrown.
util.destroyRLPXs(rlpxs)
}
resolve(undefined)
})
})
} catch (err) {
assert.fail(`An unexpected error occurred: ${err}`)
}
})
})
}, 10000)
})
5 changes: 1 addition & 4 deletions packages/devp2p/test/integration/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,7 @@
): RLPx[] {
const rlpxs = getTestRLPXs(2, maxPeers, basePort, capabilities, common)
const peer = { address: localhost, udpPort: basePort + 1, tcpPort: basePort + 1 }
rlpxs[0]['_dpt']!.addPeer(peer).catch(() => {
/* Silently catch rejections here since not an actual test error */
})
return rlpxs
return { rlpxs, peer }
}

export function destroyRLPXs(rlpxs: any) {
Expand Down Expand Up @@ -136,7 +133,7 @@
basePort = 30306,
) {
const rlpxs = initTwoPeerRLPXSetup(null, capabilities, common, basePort)
rlpxs[0].events.on('peer:added', function (peer: any) {

Check failure on line 136 in packages/devp2p/test/integration/util.ts

View workflow job for this annotation

GitHub Actions / devp2p / test-devp2p

test/integration/snap-simulator.spec.ts > Snap sync simulator tests > SNAP: send valid message 

TypeError: Cannot read properties of undefined (reading 'events')
 ❯ Module.twoPeerMsgExchange3 test/integration/util.ts:136:12
 ❯ test/integration/snap-simulator.spec.ts:26:10

Check failure on line 136 in packages/devp2p/test/integration/util.ts

View workflow job for this annotation

GitHub Actions / devp2p / test-devp2p

test/integration/snap-simulator.spec.ts > Snap sync simulator tests > SNAP: send unknown message code 

TypeError: Cannot read properties of undefined (reading 'events')
 ❯ Module.twoPeerMsgExchange3 test/integration/util.ts:136:12
 ❯ test/integration/snap-simulator.spec.ts:40:10
const protocol = peer.getProtocols()[0]
opts.sendMessage(rlpxs, protocol)
})
Expand Down
Loading