fix unit tests for mercury #12116
fix unit tests for mercury #12116
Conversation
|
I see that you haven't updated any README files. Would it make sense to do so? |
| } | ||
|
|
||
| // All feeds faced no pipeline error | ||
| // If any feed request returned an error code, return the error code with empty values, else return the values | ||
| if !allFeedsReturnedValues { | ||
| return encoding.NoPipelineError, nil, errCode, false, 0 * time.Second, reqErr | ||
| return encoding.NoPipelineError, nil, errCode, false, 0 * time.Second, nil |
There was a problem hiding this comment.
we dont need the reqErr here any more?
IIUC, this reqErr is from Mercury, and when errCode is not nil, we should expect a reqErr and return it together with the errCode?
There was a problem hiding this comment.
I didnt change this one since I am not sure. All other comments should be resolved in the most recent commit.
There was a problem hiding this comment.
reqErr is the pipeline error, it is coupled with Pipeline execution state being NoPipelineError.
There was a problem hiding this comment.
In this particular line, reqErr should be always nil since allFeedsPipelineSuccess would be true when it comes here
There was a problem hiding this comment.
Got it, I misunderstood the reqErr. It is clear to me now.
core/services/ocr2/plugins/ocr2keeper/evmregistry/v21/mercury/v02/request.go
Show resolved
Hide resolved
core/services/ocr2/plugins/ocr2keeper/evmregistry/v21/mercury/v02/v02_request_test.go
Outdated
Show resolved
Hide resolved
core/services/ocr2/plugins/ocr2keeper/evmregistry/v21/mercury/v02/v02_request_test.go
Outdated
Show resolved
Hide resolved
core/services/ocr2/plugins/ocr2keeper/evmregistry/v21/mercury/v02/v02_request_test.go
Outdated
Show resolved
Hide resolved
core/services/ocr2/plugins/ocr2keeper/evmregistry/v21/mercury/v02/v02_request_test.go
Show resolved
Hide resolved
| c.mercuryConfig.SetPluginRetry(pluginRetryKey, 0, cache.DefaultExpiration) | ||
| hc := new(MockHttpClient) | ||
|
|
||
| for i := 0; i <= 3; i++ { |
There was a problem hiding this comment.
I guess having this for loop is the same as having 3 feeds in Feeds (L630), please correct me otherwise.
There was a problem hiding this comment.
Yes i think so, this particular test can be moved to the test table, but the other tests testing multiple feed combos are not easily placed into test table
shileiwill
changed the title
|
infiloop2
merged commit a53eaff
into
auto-9004-stream-err-handler
AUTO-9078