JoinColony / colonyNetwork

Colony Network smart contracts
https://colony.io/
GNU General Public License v3.0
435 stars 106 forks source link

Migrate test framework to Hardhat #1196

Closed kronosapiens closed 6 months ago

kronosapiens commented 9 months ago

Initial drop-in migration using an external RPC node for testing

TO DO:

NOTES:

kronosapiens commented 7 months ago

Noting that the latest version of hardhat (2.21) introduced a new dependency: @nomicfoundation/edr which seems to break our build. So pinning to version 2.19 with a tilde for now.

kronosapiens commented 7 months ago

~Regarding the version check script, I'm not 100% sure. Given that the "last release" will run on truffle and the "current" version will run on hardhat we may need to update that file a few times over the next release cycle to keep the commands / paths accurate. I can keep working on it, but I'm also happy just saying that it won't work for releases which span truffle/hardhat.~

I was able to sort it out, seems to work fine now

area commented 7 months ago

Consider this approval, but with provisos:

area commented 7 months ago

The tests should correctly respond to a request for a reputation state in previous states and should correctly respond to a request for a reputation state in previous states with no proof seem to have become spotty in this PR. It doesn't seem to be the case in CI, but is definitely the case locally.

Is this only for me, or do you see it as well?

EDIT: Maybe this is something I somehow back-propagated from my rebasing attempts, checking out in a new repo doesn't show this behaviour, as far as I can see... :thinking:

kronosapiens commented 7 months ago

@area it's pretty spotty in CI for me, re-running the build usually sorts it. I've been ignoring it for the time being.

area commented 7 months ago

I've fixed them (I think), though I'm not totally sold on my fix. What was happening was that when we were explicitly calling client._miner.sync() in those tests, the client was (sometimes) still in the process of handling blocks that it had seen, and the two sync processes would clobber each other. I'm pretty sure this is only behaviour that manifests in the tests - the miner is very gun-shy about calling .sync itself, and we've written it in a way that shouldn't cause this.

To resolve, I've moved the initialisation of the client out of the beforeEach and into the tests themselves. That means the block handler isn't active while we're doing the setup of the state we want. We then call initialise, which includes a sync.

However, this is only treating the symptoms. I don't know why this PR cause this behaviour to manifest itself. My best guess is that the specific queries that the client is doing to sync take longer than they did with the Truffle RPC endpoint, and so by the time we hit the sync call on the original tests it sometimes hasn't finished. EDIT: Alternatively, everything else the tests are doing could be quicker, which would result in the same.

Are there any other newly-spotty tests you've noticed?

kronosapiens commented 7 months ago

@area that was the only one that was consistently failing