-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
FE Release 2024-09-16 #3129
FE Release 2024-09-16 #3129
Changes from 14 commits
9c01680
3eb681d
100324f
41eddc9
bf9e14b
651ce66
9504c26
2e517b3
913f0e2
da1678e
265aff4
e1d45dc
7ffafac
377a7fa
9aadad2
0357100
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,6 +82,7 @@ | |
// search for the transaction | ||
res, err := b.signozClient.SearchTraces(ctx.Context(), signoz.Last3Hr, searchMap) | ||
if err != nil { | ||
b.logger.Errorf(ctx.Context(), "error searching for the transaction: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test case for the error scenario. The error logging statement improves the visibility of potential failures. However, the static analysis tool indicates that this line is not covered by tests. Please add a test case that simulates the error scenario to ensure the logging statement is executed as expected. Do you want me to generate the test case or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
_, err := ctx.Response().Reply("error searching for the transaction") | ||
if err != nil { | ||
log.Println(err) | ||
|
@@ -290,6 +291,7 @@ | |
} | ||
} | ||
if err != nil { | ||
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test case for the error scenario. The error logging statement improves the visibility of potential failures. However, the static analysis tool indicates that this line is not covered by tests. Please add a test case that simulates the error scenario to ensure the logging statement is executed as expected. Do you want me to generate the test case or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
_, err := ctx.Response().Reply("error fetching quote request") | ||
if err != nil { | ||
log.Println(err) | ||
|
@@ -338,19 +340,23 @@ | |
err = retry.WithBackoff( | ||
ctx.Context(), | ||
func(ctx context.Context) error { | ||
txHash, err = relClient.GetTxHashByNonce(ctx, &relapi.GetTxByNonceRequest{ | ||
ChainID: rawRequest.OriginChainID, | ||
Nonce: nonce, | ||
}) | ||
txHash, err = relClient.GetTxHashByNonce( | ||
ctx, | ||
&relapi.GetTxByNonceRequest{ | ||
ChainID: rawRequest.OriginChainID, | ||
Nonce: nonce, | ||
}) | ||
if err != nil { | ||
b.logger.Errorf(ctx, "error fetching quote request: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test case for the error scenario. The error logging statement improves the visibility of potential failures. However, the static analysis tool indicates that this line is not covered by tests. Please add a test case that simulates the error scenario to ensure the logging statement is executed as expected. Do you want me to generate the test case or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
return fmt.Errorf("error fetching quote request: %w", err) | ||
} | ||
return nil | ||
}, | ||
retry.WithMaxAttempts(3), | ||
retry.WithMaxAttemptTime(15*time.Second), | ||
retry.WithMaxAttempts(5), | ||
retry.WithMaxAttemptTime(30*time.Second), | ||
) | ||
if err != nil { | ||
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | ||
Comment on lines
+354
to
+370
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Codebase Verification Add test cases for rfqRefund retry mechanism The review has revealed a lack of specific test cases for the rfqRefund function and its newly implemented retry mechanism. To ensure the reliability and correctness of this critical functionality, please consider the following actions:
Example test structure: func TestRfqRefundRetryMechanism(t *testing.T) {
// Test successful scenario
t.Run("Success", func(t *testing.T) {
// Implement test for successful rfqRefund execution
})
// Test retry scenario
t.Run("RetrySuccess", func(t *testing.T) {
// Implement test where rfqRefund succeeds after retries
})
// Test max retries exceeded
t.Run("MaxRetriesExceeded", func(t *testing.T) {
// Implement test where rfqRefund fails after max retries
})
// Test max attempt time exceeded
t.Run("MaxAttemptTimeExceeded", func(t *testing.T) {
// Implement test where rfqRefund fails due to timeout
})
} These tests will help verify the impact of the retry mechanism changes and ensure the robustness of the rfqRefund function. Analysis chainAdd test cases for the retry mechanism changes and verify the impact. The changes to the retry mechanism aim to provide a more robust handling of transient errors. However, the static analysis tool indicates that the added and modified lines are not covered by tests. Please consider the following:
Run the following script to analyze the impact of the retry mechanism changes: Do you want me to generate the test cases or open a GitHub issue to track this task? Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Analyze the impact of the retry mechanism changes.
# Test: Search for the `retry.WithBackoff` usage. Expect: Only the updated usage.
rg --type go $'retry\\.WithBackoff' -A 5
# Test: Search for test cases that simulate transient errors. Expect: At least one test case.
rg --type go $'[Tt]est.*rfqRefund' -A 20 | rg $'err.*fetching quote request'
Length of output: 32450 Script: #!/bin/bash
# Search for test files related to rfqRefund or commands.go
fd -e go test | xargs rg -i 'rfqrefund|commands\.go'
# Look for existing retry mechanism tests
rg --type go 'func Test.*Retry' -g '*_test.go'
# Check for any existing tests in the same package as commands.go
fd -e go | rg 'contrib/opbot/botmd/.*_test\.go'
Length of output: 292 Script: #!/bin/bash
# Examine the contents of commands_test.go for rfqRefund related tests
rg -i 'func test.*rfqrefund' contrib/opbot/botmd/commands_test.go
# Check for any usage of retry mechanism in the test file
rg 'retry\.WithBackoff' contrib/opbot/botmd/commands_test.go
# Look at the structure of TestRetryWithBackoff for reference
rg -A 10 'func TestRetryWithBackoff' core/retry/retry_test.go
Length of output: 501 ToolsGitHub Check: codecov/patch
|
||
_, err := ctx.Response().Reply(fmt.Sprintf("error fetching explorer link to refund, but nonce is %d", nonce)) | ||
log.Printf("error fetching quote request: %v\n", err) | ||
return | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
package metrics | ||
|
||
// HeadersToMap converts a string of headers to a map. | ||
func HeadersToMap(val string) map[string]string { | ||
return headersToMap(val) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable the
mnd
linter to detect magic numbers.The
mnd
linter is being added to the list of disabled linters, which contradicts the purpose of introducing it. To effectively detect and prevent the use of magic numbers in the codebase, themnd
linter should be enabled.Remove the following lines to enable the
mnd
linter:Committable suggestion