Skip to content

Get unit tests running again #597

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 14, 2022
Merged

Get unit tests running again #597

merged 3 commits into from
Oct 14, 2022

Conversation

scott-silver
Copy link
Contributor

@scott-silver scott-silver commented Oct 7, 2022

It looks like our unit tests stopped running after #594

If you run yarn test or yarn hardhat test locally, 0 unit tests will get run.

Our hardhat config currently defines the test path with a glob:

  paths: {
    tests: './{test,plugins/deployment_manager/test}',
  },

This worked, up until some version of Hardhat. But it appears that defining multiple paths is no longer supported (maybe never was officially supported).

Related issue: NomicFoundation/hardhat#2699

This change alters the test script to include all of our tests as arguments. And it updates the Hardhat config to use a path that Hardhat actually recognizes.

This will get tests running locally and in CI, but has the unfortunate side effect that running yarn test and yarn hardhat test will run two different sets of tests ☹️

The tests run but fail with:

/home/runner/work/comet/comet/node_modules/hardhat/internal/hardhat-network/stack-traces/solidity-errors.js:112
    return new SolidityCallSite(sourceReference.sourceName, sourceReference.contract, sourceReference.function !== undefined

🍯 This is the error that your Hardhat patch was intended to fix, correct?

Does the latest release of your forked Hardhat include the patch?

@jflatow
Copy link
Contributor

jflatow commented Oct 7, 2022

This will get tests running locally and in CI, but has the unfortunate side effect that running yarn test and yarn hardhat test will run two different sets of tests ☹️

Whoa, know why? Isn't yarn test just calling hardhat test?

The tests run but fail with:

/home/runner/work/comet/comet/node_modules/hardhat/internal/hardhat-network/stack-traces/solidity-errors.js:112
    return new SolidityCallSite(sourceReference.sourceName, sourceReference.contract, sourceReference.function !== undefined

🍯 This is the error that your Hardhat patch was intended to fix, correct?

Does the latest release of your forked Hardhat include the patch?

Yes, at least an error like that. We should give NomicFoundation/hardhat#3205 a try...

@kevincheng96
Copy link
Contributor

kevincheng96 commented Oct 7, 2022

This will get tests running locally and in CI, but has the unfortunate side effect that running yarn test and yarn hardhat test will run two different sets of tests ☹️

Whoa, know why? Isn't yarn test just calling hardhat test?

I'm guessing because yarn test now runs hardhat test ./test/*.ts ./test/**/*.ts ./plugins/deployment_manager/test/*.ts, while yarn hardhat test runs hardhat test on the tests path specified in hardhat.config.ts (which is just .test now).

This gets rid of the dependency on `ethereum-waffle` and uses a patched version of `hardhat-chai-matchers`, built from the same branch as our (updated) patched `hardhat`.
It attempts to preserve the interface we were using to match errors as much as possible, but there were a few tests that needed to be updated.
Notably, the matcher we are currently using doesn't have the ability to match custom error arguments.
We might want to switch to matching with `revertedWithCustomError` if there are improvements and/or this stabilizes in the future, but for now attempted to keep minimal changes to tests with the matcher changes.

We also move the Comet errors into the interface, in order to make things simpler/more consistent for the future, even though its not totally necessary right now.
@jflatow jflatow changed the title [draft] attempt to get unit tests running again Get unit tests running again Oct 14, 2022
@jflatow jflatow marked this pull request as ready for review October 14, 2022 17:00
@scott-silver
Copy link
Contributor Author

We also move the Comet errors into the interface, in order to make things simpler/more consistent for the future, even though its not totally necessary right now.

Does this mean we'll need a governance proposal to merge these changes?

@scott-silver
Copy link
Contributor Author

@jflatow I don't think I can approve this since I opened the original PR, but LGTM!

@jflatow jflatow merged commit 3322f4f into main Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants