Fix flaky E2E eth_streaming_test JS test
#289
Conversation
WalkthroughThe changes in Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/web3js/eth_streaming_test.js (3 hunks)
Additional comments not posted (6)
tests/web3js/eth_streaming_test.js (6)
4-4: Minor formatting improvement in object destructuring.
106-108: Validate that the assertions correctly compare the expected and actual transaction hashes.
111-111: Setting a custom timeout for the test is good practice, especially for tests involving asynchronous operations.
48-58: Similar to the previous comment, verify that the subscription to pending transactions is robust against errors and cleans up correctly.Verification successful
The search results indicate that the error handling and unsubscribe logic are consistently implemented across different subscription types in the test suite. Each subscription pattern includes error handling via the
sub.on("error", ...)method and unsubscribes correctly when the required condition is met.
File:
tests/web3js/eth_streaming_test.js
- Lines:
subBlocks.on("error", err => { rej(err) })subBlocks.unsubscribe()subTx.on("error", err => { rej(err) })subTx.unsubscribe()subLog.on("error", err => { rej(err) })subLog.unsubscribe()File:
tests/web3js/eth_streaming_filters_test.js
- Lines:
subscription.unsubscribe()The verification confirms that the subscription to pending transactions is robust against errors and cleans up correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling and unsubscribe logic are consistent across all subscription types in the test suite. # Test: Search for subscription patterns and check for error handling and unsubscribe logic. rg --type python $'subscribe' --context 10Length of output: 72
Script:
#!/bin/bash # Description: Verify that error handling and unsubscribe logic are consistent across all subscription types in the test suite. # Test: Search for subscription patterns and check for error handling and unsubscribe logic. rg --type js $'subscribe' --context 10Length of output: 8292
30-40: Ensure that the subscription to new block headers handles errors appropriately and unsubscribes correctly to avoid memory leaks.Verification successful
The search results indicate that the error handling and unsubscribe logic are present in the test suite for various subscription types. Specifically, the
unsubscribemethod is called after the required conditions are met, and errors are handled using theon("error", ...)event listener.
File:
tests/web3js/eth_streaming_test.js
- Lines:
subBlocks.unsubscribe()subTx.unsubscribe()subLog.unsubscribe()- Error Handling: Present in all cases with
subBlocks.on("error", ...),subTx.on("error", ...), andsubLog.on("error", ...).File:
tests/web3js/eth_streaming_filters_test.js
- Lines:
subscription.unsubscribe()The code appears to handle errors and unsubscribe correctly to avoid memory leaks.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling and unsubscribe logic are consistent across all subscription types in the test suite. # Test: Search for subscription patterns and check for error handling and unsubscribe logic. rg --type python $'subscribe' --context 10Length of output: 72
Script:
#!/bin/bash # Description: Verify that error handling and unsubscribe logic are consistent across all subscription types in the test suite. # Test: Search for subscription patterns and check for error handling and unsubscribe logic. rg --type js $'subscribe' --context 10Length of output: 8292
65-78: Check that the subscription to logs is correctly handling errors and unsubscribing, especially in the context of multiple asynchronous operations.Verification successful
The search results indicate that the subscription patterns in the test suite consistently handle errors and include unsubscribe logic. Here are the relevant findings:
- Error Handling: Each subscription (
subBlocks,subTx,subLog) has anon("error", ...)handler that rejects the promise with the error.- Unsubscribe Logic: Each subscription (
subBlocks,subTx,subLog) includes anunsubscribe()call when the expected condition is met.Specific Locations:
tests/web3js/eth_streaming_test.js
- Lines involving
subBlocks:let subBlocks = await ws.eth.subscribe('newBlockHeaders') subBlocks.on("error", err => { rej(err) }) subBlocks.on('data', async block => { if (blockTxHashes.length === testValues.length) { subBlocks.unsubscribe() res() } })- Lines involving
subTx:let subTx = await ws.eth.subscribe('pendingTransactions') subTx.on("error", err => { rej(err) }) subTx.on('data', async tx => { if (txHashes.length === testValues.length) { subTx.unsubscribe() res() } })- Lines involving
subLog:let subLog = await ws.eth.subscribe('logs', { address: contractAddress }) subLog.on("error", err => { rej(err) }) subLog.on('data', async (data) => { if (logTxHashes.length === testValues.length) { subLog.unsubscribe() res() } })Conclusion
The code correctly handles errors and unsubscribes from the subscriptions as expected.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling and unsubscribe logic are consistent across all subscription types in the test suite. # Test: Search for subscription patterns and check for error handling and unsubscribe logic. rg --type python $'subscribe' --context 10Length of output: 72
Script:
#!/bin/bash # Description: Verify that error handling and unsubscribe logic are consistent across all subscription types in the test suite. # Test: Search for subscription patterns and check for error handling and unsubscribe logic. rg --type js $'subscribe' --context 10Length of output: 8292
|
|
||
| const timeout = 30 // test timeout seconds | ||
|
|
||
| it('streaming of logs using filters', async() => { | ||
| setTimeout(() => process.exit(1), (timeout-1)*1000) // hack if the ws connection is not closed | ||
| it('streaming of blocks, transactions, logs using filters', async () => { |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper cleanup of WebSocket connections to avoid potential memory leaks or unintended process exits. The current test files do not show evidence of WebSocket connections being closed.
tests/web3js/eth_streaming_test.jstests/web3js/eth_rate_limit_test.jstests/web3js/simple_node_test.jstests/web3js/eth_streaming_filters_test.js
Analysis chain
Ensure proper cleanup of resources to avoid potential memory leaks or unintended process exits.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all WebSocket connections are properly closed in all test cases.
# Test: Search for WebSocket usage and ensure proper cleanup.
rg --type python $'new Web3' --context 5
Length of output: 72
Script:
#!/bin/bash
# Description: Verify that all WebSocket connections are properly closed in all test cases.
# Test: Search for WebSocket usage and ensure proper cleanup.
rg --type js 'new Web3' --context 5
Length of output: 3444
Script:
#!/bin/bash
# Description: Verify that all WebSocket connections are properly closed in the identified test files.
# Test: Search for WebSocket connection closures.
rg --type js 'ws.close' --context 5
Length of output: 35
|
@m-Peter can you please explain the description what was the change you did? with all the lining changes is a bit hard to see. |
| res() | ||
| } | ||
| }) | ||
| }) | ||
|
|
||
| // wait for subscription for a bit | ||
| await new Promise((res, rej) => setTimeout(() => res(), 300)) | ||
| await new Promise((res, rej) => setTimeout(() => res(), 1000)) |
There was a problem hiding this comment.
The 300 ms wait time seems to have been quite low.
| let doneTxs = new Promise(async (res, rej) => { | ||
| let subTx = await ws.eth.subscribe('pendingTransactions') |
There was a problem hiding this comment.
Moved the
await ws.eth.subscribeoutside the Promise closure.
| if (blockCount === testValues.length) { | ||
| await subBlocks.unsubscribe() | ||
| if (blockTxHashes.length === testValues.length) { | ||
| subBlocks.unsubscribe() |
There was a problem hiding this comment.
Removed the await from
await subBlocks.unsubscribe()as this might be causing the blocking time to increase, hence reaching the timeout limit.
There was a problem hiding this comment.
the problem with this I see, is that we now ignore any issues with unsubscribing, which would be better if test fails if there are issues. Unsubscribe shouldn't take long, so I rather increase the time a bit than not wait for result
There was a problem hiding this comment.
I took this pattern from eth_streaming_filters_test, which has never failed in CI. If we observe any problem, we can bring it back.
|
@sideninja Take this with a grain of salt, as I am not a JS ninja 😇 . But these are the main kind of changes:
My main goal was to make the test file similar to This is more or less a trial-and-error kind of change, as it is not possible to re-produce locally. I did run the job about 10 times, and did not get any failures. And this test was quite flaky, would fail about ~3 runs. |
m-Peter
force-pushed
the
fix-flaky-eth-streaming-test
branch
from
877d553 to
1470a1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
tests/web3js/eth_streaming_test.js (2)
Line range hint
8-111: Address the use ofasyncwithin Promise executors to avoid potential issues with unhandled promise rejections.The use of
asyncfunctions as promise executors in lines 31-44, 49-62, and 69-82 is flagged by static analysis as potentially problematic. This can lead to unhandled promise rejections if an error occurs before theawaitexpression. Consider refactoring these to handle errors more robustly or remove theasynckeyword if not necessary.- let doneBlocks = new Promise(async (res, rej) => { + let doneBlocks = new Promise((res, rej) => { - let doneTxs = new Promise(async (res, rej) => { + let doneTxs = new Promise((res, rej) => { - let doneAddressLogs = new Promise(async (res, rej) => { + let doneAddressLogs = new Promise((res, rej) => {Tools
Biome
[error] 31-44: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
[error] 49-62: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
[error] 69-82: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
111-111: Adjust the test timeout multiplier to reflect realistic expectations.The test timeout is set to
timeout * 1500, which might be excessively high. Consider adjusting this based on realistic network and processing delays.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/web3js/eth_streaming_test.js (3 hunks)
Additional context used
Biome
tests/web3js/eth_streaming_test.js
[error] 31-44: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
[error] 49-62: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
[error] 69-82: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
Additional comments not posted (1)
tests/web3js/eth_streaming_test.js (1)
106-108: Validate the correctness of assertions comparing arrays of transaction hashes.Ensure that the order of transaction hashes in
sentHashesand those received from subscriptions (blockTxHashes,txHashes,logTxHashes) are intended to be in the same order, asassert.deepEqualchecks for the same order of elements.
| it('streaming of logs using filters', async() => { | ||
| setTimeout(() => process.exit(1), (timeout-1)*1000) // hack if the ws connection is not closed | ||
| it('streaming of blocks, transactions, logs using filters', async () => { | ||
| setTimeout(() => process.exit(1), (timeout - 1) * 1000) // hack if the ws connection is not closed |
There was a problem hiding this comment.
Consider handling the potential unclean exit due to process.exit(1).
Using process.exit(1) as a timeout mechanism can lead to unclean exits and unhandled state. It's better to use proper test timeouts or cleanup mechanisms.
| if (blockCount === testValues.length) { | ||
| await subBlocks.unsubscribe() | ||
| if (blockTxHashes.length === testValues.length) { | ||
| subBlocks.unsubscribe() |
There was a problem hiding this comment.
Ensure proper cleanup of subscriptions to avoid potential memory leaks.
The unsubscribing from events is handled within the data event handlers. This might lead to cases where unsubscribing does not occur if no data events are received. Consider adding a timeout or a final block to ensure unsubscribing happens.
Also applies to: 58-58, 78-78
| @@ -1,12 +1,12 @@ | |||
| const conf = require('./config') | |||
| const helpers = require('./helpers') | |||
| const { assert } = require('chai') | |||
| const {Web3} = require("web3"); | |||
| const { Web3 } = require("web3"); | |||
There was a problem hiding this comment.
Ensure consistent spacing in object destructuring.
- const { Web3 } = require("web3");
+ const {Web3} = require("web3");Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { Web3 } = require("web3"); | |
| const {Web3} = require("web3"); |
| let blockCount = 0 | ||
| let blockHashes = [] | ||
| let blockTxHashes = [] | ||
| let subBlocks = await ws.eth.subscribe('newBlockHeaders') |
There was a problem hiding this comment.
why you think this is better?
There was a problem hiding this comment.
I took this pattern from eth_streaming_filters_test, which has never failed in CI. If we observe any problem, we can change it back.
|
@m-Peter are you able to re-run the CI couple of time so you can see if things are better? |
|
@sideninja Yes, I did run the CI quite some times on Saturday. I wasn't able to produce a failure. |
Closes: #283
Description
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit