Umbra js hardhat tests & coverage

[wildmolasses]

Resolves #104

Converted umbra-js test suite to use hardhat node/provider. Added coverage package.

Coverage can be run via

cd umbra-js
yarn coverage

[mds1]

This is awesome!! Great work! Left a few small comments, and also have a question for you here:

First, check out the screenshot of my coverage results:

Notice how umbra-js/src/inner has zero coverage. That's because this folder and the contract.ts file it references does not exist! It used to, but we deleted it some time ago. So where is istanbul picking up this folder/file from?

Edit: turns out the answer was that folder/file was cached in the build directory. Running yarn clean && yarn from the root then re-running coverage fixed it. More info at istanbuljs/nyc#644

[mds1]

Hey @wildmolasses one more comment—can you add the coverage output folders/files to .prettierignore?

[mds1]

Looks great! Two last things before merging:

1. Can we update yarn clean to delete the coverage folders? It seems it only deletes the build folder but not .nyc_output or coverage
2. Can you update the READMEs to explain how to setup the repo from a fresh clone? I'm not sure of the relationship between .env files at the monorepo root vs. .env files in all the subfolders, which variables they share vs. which are unique to a workspace, etc.

[wildmolasses]

Looks great! Two last things before merging:

1. Can we update yarn clean to delete the coverage folders? It seems it only deletes the build folder but not .nyc_output or coverage

✅ yarn clean now deletes the coverage-related stuff

2. Can you update the READMEs to explain how to setup the repo from a fresh clone? I'm not sure of the relationship between .env files at the monorepo root vs. .env files in all the subfolders, which variables they share vs. which are unique to a workspace, etc.

✅ as @mds1 and I just discussed, we will keep .env files in their subpackage folders for now. Added a bit more instruction to the README suggesting this.

⚠️ Note that the "contracts" env variable INFURA_API_KEY has changed to INFURA_ID.

[apbendi]

I'm officially extremely confused about what's going on here. The PR is showing two commits that don't seem to exist on the branch:

When using the latest branch Github will give me, I.e. 15df72, I get the following error trying to run yarn test

@umbra/contracts: $ concurrently --success first --kill-others "hardhat node &>/dev/null" "hardhat test --network localhost"
@umbra/contracts: [1] Please set your INFURA_ID in a .env file
@umbra/contracts: [1] Creating Typechain artifacts in directory typechain for target ethers-v5
@umbra/contracts: [0] hardhat node &>/dev/null exited with code 1
@umbra/contracts: --> Sending SIGTERM to other processes..
@umbra/contracts: [1] hardhat test --network localhost exited with code SIGTERM
@umbra/contracts: error Command failed with exit code 1.
@umbra/contracts: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

This looks like the error when I have another network taking that port, but that isn't the case currently.

[mds1]

Locally I also see 15d724a as the last commit, so it's odd we're both seeing that. Viewing the branch directly also shows 15d724a as the last commit. I see you also tried changing the PR target branch to help update it but it seems that didn't work... 🤔

@apbendi Regarding that error, you might recall that was the issue I kept having! I thought it was from lagging hardhat processes not closing on my machine for some reason, but maybe it's a Hardhat bug? Usually I work around this by running tests "manually", e.g. cd contracts && yarn hardhat node in one terminal then cd contracts && yarn test in the other

@wildmolasses In contracts/hardhat.config.ts on line 3 it looks like we try to import ../.env, but based on your last comment shouldn't it just be ./.env? The former tries to look in the project root even though above we said "we will keep .env files in their subpackage folders for now". Due to the commit issues, I confirmed this using the latest commit on the branch listed here which it says is 15d724a.

[mds1]

@wildmolasses By any chance did you change the order of commits in the rebase before force-pushing? My best guess right now is you might have accidentally changed the order improperly, which is messing things up. The reason I think this is because this PR shows Fix provider config for hardhat compatibility, hardcode a test mnemonic, commit d8d7ade as the most recent commit, but I actually pushed that commit a week or so ago when helping you fix the Umbra.test.ts issues

[apbendi]

Regarding that error, you might recall that was the issue I kept having! I thought it was from lagging hardhat processes not closing on my machine for some reason, but maybe it's a Hardhat bug?

This only happens for me on this branch. Any other branches the tests run fine. I wonder if it's related to the error about the infura key in .env? I have a .env, with the infura key set, in both the root repo dir and the contracts dir, so I'm not sure why I'm seeing that error.

[apbendi]

this PR shows Fix provider config for hardhat compatibility, hardcode a test mnemonic, commit d8d7ade as the most recent commit, but I actually pushed that commit a week or so ago when helping you fix the Umbra.test.ts issues

Actually, this might just be a github bug. It seems the PR screen is just showing them in the wrong order for some reason.

[wildmolasses]

hey guys!

By any chance did you change the order of commits in the rebase before force-pushing? My best guess right now is you might have accidentally changed the order improperly, which is messing things up.

No! all I did was reword an earlier commit to remove its "wip" prefix.

Actually, this might just be a github bug. It seems the PR screen is just showing them in the wrong order for some reason.

this is my hypothesis too.

In contracts/hardhat.config.ts on line 3 it looks like we try to import ../.env, but based on your last comment shouldn't it just be ./.env?

Thanks, this is a bug!

This only happens for me on this branch. Any other branches the tests run fine. I wonder if it's related to the error about the infura key in .env? I have a .env, with the infura key set, in both the root repo dir and the contracts dir, so I'm not sure why I'm seeing that error.

@apbendi can you check that your contracts .env is set with an INFURA_API_KEY INFURA_ID?

FYI, the workspace root .env was not implemented, so we can delete that .env file.

@apbendi I also saw a comment from you that seemed to get deleted concerning @openzeppelin/test-environment. That seems to have lingered in contracts/test/utils.js. Let me remove it.