Improve client test coverage#1024
Conversation
ylg-avalabs
requested review from
batconjurer,
bernard-avalabs,
geoff-vball,
iansuvak,
mdelle1 and
michaelkaplan13
| // TestProposerVMAPI_CombinedQueryParamsAndHeaders verifies both work together | ||
| func TestProposerVMAPI_CombinedQueryParamsAndHeaders(t *testing.T) { |
There was a problem hiding this comment.
Is there a way that headers and query params interact that we need to test them together?
geoff-vball
left a comment
geoff-vball
left a comment
There was a problem hiding this comment.
Just a few nits. They should also be applied to the other test in the file. Otherwise lgtm :)
| logger := logging.NoLog{} | ||
| _, err := NewDestinationClient(logger, &destinationBlockchain, time.Minute) |
There was a problem hiding this comment.
We should just inline short declarations that are only used once.
| logger := logging.NoLog{} | |
| _, err := NewDestinationClient(logger, &destinationBlockchain, time.Minute) | |
| _, err := NewDestinationClient(logging.NoLog{}, &destinationBlockchain, time.Minute) |
| client, err := NewDestinationClient(logger, &destinationBlockchain, time.Minute) | ||
| require.NoError(t, err) | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
| ctx := context.Background() | |
| ctx := t.Context() |
| mu.Lock() | ||
| defer mu.Unlock() |
There was a problem hiding this comment.
I think this mutex is just ensuring that the handler func has finished running? But ethClient.SuggestGasTipCap(ctx) won't finish until the handlerfunc is finished, so I don't think we need it.
|
@ylg-avalabs I left some feedback here that should be pretty easy to address, and then we can get this merged in :) |
| "X-API-Key": "secret-key", | ||
| } | ||
|
|
||
| var mu sync.Mutex |
There was a problem hiding this comment.
This mutex isn't needed
| "api-key": "secret-key-789", | ||
| } | ||
|
|
||
| var mu sync.Mutex |
There was a problem hiding this comment.
This mutex isn't needed
| AccountPrivateKeys: []string{"56289e99c94b6912bfc12adc093c9b51124f0dc54ac7a766b2bc5ccf558d8027"}, | ||
| } | ||
|
|
||
| logger := logging.NoLog{} |
There was a problem hiding this comment.
It would be cleaner to inline logging.NoLog{} on the line below (and do the same for the other functions in this file)
batconjurer
left a comment
batconjurer
left a comment
There was a problem hiding this comment.
It seems to me that there is a lot of code repetition. It would be nice to try and abstract the setup into separate functions.
geoff-vball
left a comment
geoff-vball
left a comment
There was a problem hiding this comment.
It might be worthwhile to consolidate these tests where applicable re: Jacob's comments, but this PR has been languishing for a while, and it's just test code, so I'm fine merging it as is if you'd like
Yeah, lets get it merged. |
3 participants
Why this should be merged
How this works
How this was tested
How is this documented