Remove async from describe blocks and add missing await in tests

[fvictorio]

When testing the latest version of Hardhat in the OZ repo, we noticed that some of the tests were suddenly failing. The problem was caused by a combination of some missing or extra async/await keywords. Mainly: the functions of some describe blocks had the async keyword (and they shouldn't), and some async assertions were missing the await keyword. This, combined with the different performance characteristics of the latest HH version, surfaced some issues that weren't showing up before just by coincidence. I fixed that in fe64f5c.

After that, some other tests started failing. Most of them are related to event assertions that either had the wrong order of arguments, or had a missing argument. These were actually failing before, but since they weren't being awaited, they were floating rejected promises that didn't make the tests fail. I fixed that in 258a203.

Now there is only one test failing, the grants role to the user test in AccessManager.test.js. This is asserting that a RoleAccessRequested was emitted but, as far as I can tell, that event doesn't exist anymore. I couldn't find an equivalent one so I just left it as it was; I'm sure you can fix it faster than me.

PR Checklist

This PR only affects tests, so no docs change or changeset entry is needed.

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

⚠️ No Changeset found

Latest commit: 1be335c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[fvictorio]

I forgot to mention:

• The wrong async keywords in describe functions can be prevented with this rule.
• The missing await keywords are harder. Ideally you'd use typescript and the no-floating-promises rule, but I understand that that would be a huge undertaking.

[ernestognw]

Hey @fvictorio, thank you for raising this. After we migrated tests to Ethers I think this was one of our major worries (getting ghost tests).

For the record, this comes from this PR. I guess the oversight came from the previous order of the arguments but I can't explain how RoleAccessRequested appeared.

I fixed the tests, and I think we'll need to discuss adding the no-async-describe rule (@Amxx ). Unfortunately for the await cases, I don't think we're supporting Typescript soon, not only is it a big commitment but is increasing the dependency surface, and the benefit comes only for testing purposes.

[ernestognw]

LGTM, we need to discuss the options to avoid it happening again, but the PR is good to go rn.

[fvictorio]

Thanks for the review!

[fvictorio]

Maybe you can consider adding this somewhere in the tests setup:

process.on("unhandledRejection", (reason, promise) => {
  throw promise;
});

[Amxx]

Changes to the test files look good.

Is the hardhat update necessary, it comes with a big package-lock.json diff ...

[ernestognw]

@fvictorio we see a lot of changes to the package-lock.json, is there any reason for upgrading Hardhat?

We may do this in a future PR but I wonder if we need it now

[fvictorio]

No, no need to update it now. You might get a 10-15% performance improvement, but there isn't a strong reason for you to upgrade. I can rebase that commit out if you want.

[ernestognw]

No, no need to update it now. You might get a 10-15% performance improvement, but there isn't a strong reason for you to upgrade. I can rebase that commit out if you want.

Yeah I think we'd prefer to keep this PR short. We still want to upgrade for next hardfork though

[fvictorio]

Done!

[ernestognw]

Maybe you can consider adding this somewhere in the tests setup:

process.on("unhandledRejection", (reason, promise) => {
  throw promise;
});

I tried this suggestion and a couple more of issues came up. Thanks! I'm updating

[ernestognw]

Thanks for all your work @fvictorio 🙏🏻
I created an issue for the ESLint rule so we can discuss it later: #4943

Rn it's looking good again, let's wait on @Amxx approval.

[Amxx]

Moved

process.on('unhandledRejection', reason => {
  throw new Error(reason);
});

to a dedicated file + added a missing await (are there any more?)

[ernestognw]

to a dedicated file + added a missing await (are there any more?)

Right, the one you spotted is a tricky one given is expecting no changes. The test should make sure the function is called before asserting, though. I don't expect other tests to be in similar cases.