Restructure tests #93
Conversation
...acts/vendor/starkware-libs/starkgate-contracts/src/starkware/solidity/tokens/ERC20/ERC20.sol
Outdated
Show resolved
Hide resolved
| `${__dirname}/../../../node_modules/internals-starkgate-contracts/artifacts/0.0.3/eth/${name}.json`, | ||
| ) | ||
| .toString('ascii'), | ||
| ) | ||
| } | ||
|
|
||
| export const loadOpenzepplinContract = (name: string): any => { | ||
| return json.parse( | ||
| fs | ||
| .readFileSync( | ||
| `${__dirname}/../../../node_modules/@openzeppelin/contracts/build/contracts/${name}.json`, | ||
| ) | ||
| .toString('ascii'), | ||
| ) | ||
| } | ||
|
|
||
| export const loadSolidityContract = (name: string): any => { | ||
| return json.parse( | ||
| fs | ||
| .readFileSync( | ||
| `${__dirname}/../../../contracts/artifacts/src/chainlink/solidity/mocks/${name}.sol/${name}.json`, |
There was a problem hiding this comment.
It's slightly unbelievable that we need to load contracts like this - very brittle.
For this PR please find all instances of functions like this (ctrl + F 'readFileSync') and please rename all these load functions to start with loadContract_ prefix so they are easily discoverable.
We'll need to go in as as a separate effort and figure out proper contract loading for both Cairo and Solidity.
There was a problem hiding this comment.
Opened a GH Issue on this:
krebernisak
force-pushed
the
restructure-tests
branch
from
45f6dfd
to
c0ce6ee
Compare
krebernisak
force-pushed
the
restructure-tests
branch
from
1fef8e5
to
1db4a8e
Compare
| test-integration-gauntlet: build-ts env-devnet-hardhat-down | ||
| cd packages-ts/starknet-gauntlet/ && \ | ||
| yarn test | ||
| cd packages-ts/starknet-gauntlet-argent/ && \ | ||
| yarn test | ||
| cd packages-ts/starknet-gauntlet-cli/ && \ | ||
| yarn test | ||
| cd packages-ts/starknet-gauntlet-example/ && \ | ||
| yarn test | ||
| cd packages-ts/starknet-gauntlet-multisig/ && \ | ||
| yarn test | ||
| cd packages-ts/starknet-gauntlet-ocr2/ && \ | ||
| yarn test | ||
| cd packages-ts/starknet-gauntlet-oz/ && \ | ||
| yarn test | ||
| cd packages-ts/starknet-gauntlet-starkgate/ && \ | ||
| yarn test |
There was a problem hiding this comment.
Why not rely on the toplevel yarn test which uses the Jest config to run all the tests? I personally only use that and ignore the Makefile
There was a problem hiding this comment.
The version before this PR just ran Jest tests via root package.json test script.
The problem was not all workspace packages use Jest for testing - our contracts, example project, and 2x integration projects (multisig, starkgate) use hardhat + mocha, so they wouldn't be run.
Then I updated the root package.json test script to iterate over all projects and run their test scripts via "yarn workspaces run test" - but now the problem is Hardhat setup tests expect external network to be started already, and Gauntlet Jest tests run their own network (Integrated Devnet) on beforeAll. This was now a conflict because Integrated Devnet wouldn't start successfully if docker container was already running on the same 5050 port.
So for now I've split the two category of tests as different target, and different CI workflows. Now there is no infra conflict, and all tests are finally ran.
This is enough for this PR I'd say, and the plan for the next one is to restructure how do we run local env in tests as dependencies. Every tests suite should start its own network programatically in test setup, and tear it down when finished. At that point we can just run all tests at once via "yarn workspaces run test" and they would be even more isolated.
There was a problem hiding this comment.
tldr; I consider this a temp workaround, and will try to remove it as test runs are restructured to be idempotent.
There was a problem hiding this comment.
Extracted a GH Issue:
| "ts-node": "^10.4.0", | ||
| "typescript": "^4.5.2" |
There was a problem hiding this comment.
Aren't these required for tests?
There was a problem hiding this comment.
They are declared as part of <root>/package.json (just once, single version) and used by the whole yarn workspace. No need to redeclare them here, if a custom version is not required.
| @@ -3,18 +3,38 @@ import { StarknetContract, Account } from 'hardhat/types/runtime' | |||
| import { uint256 } from 'starknet' | |||
| import { toBN } from 'starknet/utils/number' | |||
| import { TIMEOUT } from '../../../constants' | |||
| import { expectInvokeError } from '../../../../test/utils' | |||
There was a problem hiding this comment.
Maybe We can write it back to import { expectInvokeError } from '../../../utils'
| @@ -1,38 +1,58 @@ | |||
| import { constants, ec, encode, hash, number, uint256, stark, KeyPair } from 'starknet' | |||
There was a problem hiding this comment.
Don't need all of it we can change it to import { constants, encode, number} from 'starknet'
Test for starkgate bridge with our link_token