Hardhat tests: use loadFixture() #89
Conversation
bpierre
changed the title
This is a test of migrating to loadFixture() using TroveManagerTest only for now. The migration consists of converting beforeEach() into a fixture, which is recommended by Hardhat mostly for performance reasons [1]. On my computer, this test went from ~54s to ~18s (~67% faster). Before: pnpm test test/TroveManagerTest.js 53.90s user 6.48s system 145% cpu 41.495 total After: pnpm test test/TroveManagerTest.js 17.98s user 1.81s system 138% cpu 14.309 total [1] https://hardhat.org/hardhat-runner/docs/guides/test-contracts#using-fixtures
…Test This ocmmit also adds the createDeployAndFundFixture() utility.
bpierre
changed the title
This commit also adds the possibility to pass mocks to deployLiquityCoreHardhat() and deployBoldToken().
But still using fixtures.
I suspect this was the root cause for the finicky tests.
| describe("Test", async () => { | ||
| testCorpus(); | ||
| }); | ||
| testCorpus(); |
There was a problem hiding this comment.
I removed the describe("test") but kept the testCorpus() call to not mess up with the indentation in this PR.
bpierre
changed the title
It’s probably better to not rely on `accounts[0]` to know if all accounts have been funded, since `accounts` is dynamic.
| // const areAccountsAlreadyFunded = await token | ||
| // .balanceOf(accounts[0]) | ||
| // .gte(_1e36Str); | ||
| // if (areAccountsAlreadyFunded) return; |
There was a problem hiding this comment.
It’s probably better to not rely on accounts[0] to know if all accounts have been funded, since accounts is dynamic.
| return accounts.forEach(async (account) => { | ||
| await token.mint(account, _1e36Str); | ||
| }); | ||
| return Promise.all( | ||
| accounts.map((account) => ( | ||
| token.mint(account, String(10n ** 36n)) | ||
| )) | ||
| ); |
There was a problem hiding this comment.
Probably the cause of the remaining finicky tests. I think what was happening was that the tests were getting executed before the accounts funding had finished, which would also explain why it became more frequent once the tests execution started to accelerate.
There was a problem hiding this comment.
Good catch! If one wants to run async functions in sequence the old fashioned way, it has to be a regular loop:
for (const account of accounts) {
await token.mint(account, _1e36Str);
}(Since we don't use the return values anyway).
I also like Promise.all() more. There's a subtle difference, but for us it doesn't matter. If we were on a real chain, the Promise.all() version would try to fire a whole bunch of network requests all at once, which might not be a good idea. Doesn't matter in hardhat though.
There was a problem hiding this comment.
Yes true 😅, on a real network we could have some utility to limit the number of promises running concurrently.
|
How come there are fewer tests than on latest main? Main: This PR: |
Excellent question 😅 After investigating it was caused by some missing imports, and hardhat seems to silence runtime errors for some reason? 🤔 It’s fixed now, thanks for noticing it 🙏 77b757b |
Some skipped test cases weren't being counted towards the total. Although they're currently skipped, it's better to fix this or we might forget about them in the future.
This PR migrates all the Hardhat tests to take advantage of
loadFixture(), resulting in a faster execution.It also:
On my machine, running the entire test suite went from 11mn and 24s to 39s (~18x faster).
On the GitHub Action, we seem to be going from ~11mn to ~3mn for the tests, and from ~21mn to ~11mn for the test coverage.