Closed area closed 3 months ago
Looks like we weren't out of the woods with spotty tests, as a build here threw with the other side closed
error I was seeing a lot before monkeypatching truffle-contract
.
We thought we solved that when we stopped the node being spammed, so I've taken a similar approach here and replaced our use of JsonRPCProvider
from ethers with StaticJsonRPCProvider
which will approximately halve the number of requests we're making again.
My hypothesis here is that by having fewer eth_chainId
requests, we're more sensitive to race conditions that have hitherto gone unnoticed.
In terms of the changes to the tests, in these two tests we see them fail intermittently waiting for events on-chain that happen. That suggests that the event is firing before the promise is set up; as a result, I'm trying to rework these tests to use more precise timestamps so that we don't accidentally end up letting a client confirm a hash before we set up the promise for it.
I realise we could just move when we set up the promise, but I think that's just patching over the underlying problem, which I'd prefer to resolve in a manner that (hopefully) means that when writing tests like this in the future we don't stumble across this issue.
Ok, this looks good to me. Let me know when you're ready for another review.
These two most recent commits:
processingDelay
to 1 everywhere in the auto
test suite. We were already doing this a bit, and it makes sense to do it everywhere. This only exists to protect against block reorgs, which aren't happening in these tests, and it means we aren't editing the variable on the miner tracking it on the fly, which I could believe could lead to race conditionsgetMiningCycleCompletePromise
and getWaitForNSubmissionsPromise
. I'm all but certain the timestamps are correct now, so I'm left to conclude that even if the promise is being set up, the handler is not before the event is emitted on chain. This change makes sure that the handler is added (at least, if we can trust what ethers is telling us)I think it's unlikely the first one was much of a factor, but I'm quite optimistic that the second one here is what we've been chasing down :crossed_fingers:
EDIT: ðŸ˜
EDIT 2: Actually, this implementation doesn't guarantee number 2 is the case...
I've reworked the approach to getWaitForNSubmissionsPromise
and getMiningCycleCompletePromise
to not use the event emitters at all, but to query for the events post-hoc. I think this should be much more reliable, without jumping through hoops making sure that the event handler is set up, which would have been needed to fix my original approach. It does result in more requests to the node, but I think that's unavoidable
We keep having issues with
package-lock.json
, this helps guards against this a bit. Note we don't do anything with the resulting images, we just check they build okay.