chore(tests): refactor tests/utils/request_utils.go

[qdm12]

Changes

• Inject context to PostRPC and PostRPCWithRetry
• Swap method and url arguments for PostRPC and PostRPCWithRetry
• Remove some global variables
• Wrap all possible errors
• PostRPCWithRetry retries until context is canceled
• Keep 1s timeout for RPC calls

Tests

Existing tests suites passing

Issues

• tests/utils: refactor RPC request functions to be context based #2386

Primary Reviewer

[codecov]

Codecov Report

Merging #2385 (13fcf3e) into development (11f6c1c) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           development    #2385      +/-   ##
===============================================
- Coverage        58.50%   58.47%   -0.03%     
===============================================
  Files              214      214              
  Lines            28004    28004              
===============================================
- Hits             16383    16375       -8     
- Misses            9963     9971       +8     
  Partials          1658     1658              

Flag	Coverage Δ
unit-tests	58.47% <ø> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dot/network/inbound.go	85.96% <0.00%> (-14.04%)	⬇️
dot/state/block_notify.go	81.90% <0.00%> (-8.58%)	⬇️
dot/network/block_announce.go	59.32% <0.00%> (-5.09%)	⬇️
dot/network/notifications.go	66.07% <0.00%> (-1.43%)	⬇️
dot/network/connmgr.go	89.04% <0.00%> (-1.37%)	⬇️
dot/network/service.go	56.84% <0.00%> (-0.47%)	⬇️
dot/sync/chain_sync.go	53.42% <0.00%> (-0.31%)	⬇️
lib/runtime/wasmer/imports.go	48.40% <0.00%> (-0.05%)	⬇️
dot/network/host.go	65.37% <0.00%> (+1.06%)	⬆️
lib/blocktree/blocktree.go	54.71% <0.00%> (+1.08%)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11f6c1c...13fcf3e. Read the comment docs.

[timwu20]

could this function not just accept a time.Duration for timeout and handle context creation and cancellation within this function? Doesn't look like the contexts are used for anything other than timeout functionality.

[qdm12]

Yes for now, but the idea I have at the back of my head is to use it with cancel() in the future once it's more event-driven.

Context offer the flexibility of timeouts + event driven cancellation, and inherit from each other, so we should really be using them in e2e tests instead of timers/signal channels which would make a mess of a code especially regarding 'cascaded cancelation'.

Also contexts are part of the std library, and most standard library use them like http.NewRequestWithContext or exec.CommandContext

[qdm12]

Further down the line in my branches, this context is actually used to cancel operations, for example if a node crashes unexpectedly.

[timwu20]

I'd argue that we shouldn't be using context for cancellation. Dave Cheney wrote about it in his blog.

The documentation for the context package strongly recommends that context.Context is only for request scoped values:

With regards to cascading cancellation, you can just have a cancel channel that is closed and can be listened on by multiple goroutines.

I know using contexts for cancellation is a common pattern. It's something to keep an eye on for Go 2. They may remove the cancellation from the context package.

[qdm12]

Interesting blog post, and I 100% agree context values are the fruit of demoniac developers 👿 (except they're 100% needed for i.e. a server metrics given restrictive signatures of handlers).

It's also sad they didn't figure out how to wait for a goroutine to finish on context cancellation, forcing to still have another signal/error channel to wait.

Now for the closing of a signal channel, there are 3 limitations I can think of:

• Handling timers to close a channel is painful (always need to check to empty the timer channel as well), and even more when nesting them.
• You cannot close a channel multiple times, so you would have to pretty much re-invent contexts for this (check if the channel is closed etc.)
• The standard library accepts contexts to cancel IO operations, but not close channels and often have no easy Stop method. One could use a signal channel to cancel a context down the callers, but it feels wrong to me.

[EclesioMeloJunior]

lgtm! just a small change