deps(testing) - consolidate testing frameworks across the project #819
Conversation
…he CI does not error out
…ve enough time to run in the CI
… json files are read correctly
| @@ -8,9 +8,10 @@ | |||
| "bootstrap": "lerna bootstrap", | |||
| "build": "lerna run build", | |||
| "lint": "eslint './**/**/*.ts'", | |||
| "lint-fix": "eslint './**/**/*.ts' --fix" | |||
| "lint-fix": "eslint './**/**/*.ts' --fix", | |||
| "test": "lerna run test --ignore maci-common --ignore maci-server --ignore maci-integrationtests --ignore maci-cli" | |||
There was a problem hiding this comment.
Why ignore maci-cli? Is that because you'll plan to refactor that as part of #794?
There was a problem hiding this comment.
I also wonder how intuitive it is to ignore integration tests 🤔 but I think it makes sense
There was a problem hiding this comment.
Basically here I'm trying to run only unit test, hence why cli and integration tests are ignored.
Running them in this command would involve first spawning a local hardhat node
There was a problem hiding this comment.
Running them in this command would involve first spawning a local hardhat node
But doesn't running contract tests also spin up a hardhat node?
There was a problem hiding this comment.
It does, however because we run them using hardhat directly (hardhat test x.test.ts) there is no need to pre spawn a node, it just does it automatically and kills it once done
| @@ -27,7 +27,7 @@ jobs: | |||
| id: check_changes | |||
| run: | | |||
| CHANGED_FILES=${{ steps.get-changed-files.outputs.all }} | |||
| if echo "$CHANGED_FILES" | grep -q "circuit" && ! echo "$CHANGED_FILES" | grep -q ".github/workflows/circuit-build.yml"; then | |||
| if echo "$CHANGED_FILES" | grep -q "\.circom"; then | |||
There was a problem hiding this comment.
Oh yea sorry for not mentioning in the description! Basically this change is so that the e2e test only run when the actual circom files change, not any files in the circuits folder as it was until this commit. I was planning to introduce this with another PR but got carried away by not wanting to wait on circuits compilation for this PR
README.md
Outdated
|
|
||
| ```bash | ||
| cd crypto | ||
| npm run test | ||
| ``` | ||
|
|
||
| <!-- TODO: confirm if true --> |
There was a problem hiding this comment.
@ctrlc03 is this necessary? Seems I'm able to run the full suite e.g. of contracts tests without nonce errors...
There was a problem hiding this comment.
First, start a Hardhat instance in a separate terminal:
cd contracts npm run hardhat
Is this needed? I'm able to just run npm run test within contracts & it seems to automatically start up Hardhat
There was a problem hiding this comment.
yea that's how it's supposed to work (hardhat does start itself) so yea feel free to remove. In general most of these README are outdated (wondering if it might better to address all in one PR?)
There was a problem hiding this comment.
In general most of these README are outdated (wondering if it might better to address all in one PR?)
I'm alright with that, though generally I think best to update documentation at the same time as the code is updated (ideally in the same PR), because:
- that tends to make it easier for PR creator, given the specific info & context is fresh in their mind
- it also makes it easier for the reviewer, so they can easily understand what's changing
- it avoids the issue of having this big chore for someone to pick up at the end of this refactoring effort. Not to mention if the chore drops as a priority, then everything is a mess 😄
There was a problem hiding this comment.
Noted! I'll make sure from now on all doc updates are included in the PR 🙂
|
Great work! |
This PR removes usage of JEST in favour of Mocha + Chai -> given we have multiple packages in this monorepo and both circuits and contracts tests use mocha (circom_tester and hardhat)