Print command error when failing, not output #321
Conversation
|
Warning Review failedThe pull request is closed. WalkthroughThe updates involve improvements to test handling across three files. In Changes
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: 0
Outside diff range and nitpick comments (1)
tests/helpers.go (1)
Line range hint
51-51: Security Issue: Hardcoded API Keys Detected.Hardcoding API keys in the source code is a significant security risk. These keys can be accessed by anyone who has access to the repository, potentially leading to unauthorized access to services.
- servicePrivateKey = "61ceacbdce419e25ee8e7c2beceee170a05c9cab1e725a955b15ba94dcd747d2" - eoaTestPrivateKey = "f6d5333177711e562cabf1f311916196ee6ffc2a07966d9d4628094073bd5442" + // Remove hardcoded keys. Use environment variables or a secure vault service.Consider using environment variables or a secure vault service to manage sensitive information securely. Would you like assistance in implementing this change?
Also applies to: 54-54
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/helpers.go (1 hunks)
Additional context used
Gitleaks
tests/helpers.go
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (1)
tests/helpers.go (1)
194-194: Improve error handling in test execution.Changing the error reporting to use
err.Error()instead ofstring(out)is a good practice as it focuses on the actual error message rather than potentially misleading output. This aligns with the PR's objectives to improve clarity in error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
tests/web3js/eth_streaming_test.js (3)
Line range hint
33-46: Removeasyncfrom Promise executor functions to prevent potential unhandled rejections.Using
asyncfunctions as Promise executors can lead to unhandled promise rejections. This can make error handling and debugging more difficult.- let doneBlocks = new Promise(async (res, rej) => { + let doneBlocks = new Promise((res, rej) => {
Line range hint
51-64: Removeasyncfrom Promise executor functions to improve error handling.Similar to the previous comment, using
asyncin Promise executors is not recommended. It should be refactored to handle asynchronous operations properly without risking unhandled rejections.- let doneTxs = new Promise(async (res, rej) => { + let doneTxs = new Promise((res, rej) => {
Line range hint
71-84: Refactor Promise executor to removeasynckeyword.Continuing the pattern from earlier comments, removing
asyncfrom Promise executors is necessary to ensure that errors are handled correctly and not left uncaught.- let doneAddressLogs = new Promise(async (res, rej) => { + let doneAddressLogs = new Promise((res, rej) => {tests/helpers.go (1)
Line range hint
51-51: Remove hardcoded API key to enhance security.Hardcoded secrets pose a significant security risk, especially in public repositories. Consider using environment variables or secure vault solutions to manage secrets.
- servicePrivateKey = "61ceacbdce419e25ee8e7c2beceee170a05c9cab1e725a955b15ba94dcd747d2" + servicePrivateKey = os.Getenv("SERVICE_PRIVATE_KEY")
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- tests/helpers.go (1 hunks)
- tests/web3js/eth_streaming_filters_test.js (2 hunks)
- tests/web3js/eth_streaming_test.js (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/web3js/eth_streaming_filters_test.js
Additional context used
Biome
tests/web3js/eth_streaming_test.js
[error] 33-46: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
[error] 51-64: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
[error] 71-84: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
Gitleaks
tests/helpers.go
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (1)
tests/helpers.go (1)
Line range hint
54-54: Remove hardcoded private key to prevent security vulnerabilities.Similar to the previous comment, storing private keys in the source code can lead to security breaches. Use secure storage solutions instead.
[ISSURE]- eoaTestPrivateKey = "f6d5333177711e562cabf1f311916196ee6ffc2a07966d9d4628094073bd5442" + eoaTestPrivateKey = os.Getenv("EOA_TEST_PRIVATE_KEY")
| @@ -191,7 +191,7 @@ func executeTest(t *testing.T, testFile string) { | |||
| var exitError *exec.ExitError | |||
| if errors.As(err, &exitError) { | |||
| if exitError.ExitCode() == 1 { | |||
| require.Fail(t, string(out)) | |||
| require.Fail(t, err.Error()) | |||
There was a problem hiding this comment.
can we print both, because I believe the output will have information from the execution
There was a problem hiding this comment.
out, err := cmd.CombinedOutput()
t.Log(string(out))The output is already printed, outside the condition.
| setTimeout(() => process.exit(1), (timeout-1)*1000) // hack if the ws connection is not closed | ||
| // hack if the ws connection is not closed | ||
| // TMP: disable | ||
| // setTimeout(() => process.exit(1), (timeout - 1) * 1000) |
There was a problem hiding this comment.
I think with mocha set timeout we could remove this completely
There was a problem hiding this comment.
Good catch 👍
Removed in 7527958
Description
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Bug Fixes
Tests
eth_streaming_filters_test.jsfrom 20 to 30 for more robust testing.eth_streaming_filters_test.jsandeth_streaming_test.jsto prevent premature termination during tests.