feature: add initial integration tests#27
feature: add initial integration tests#27AlonzoRicardo wants to merge 12 commits intotetherto:developfrom
Conversation
package.json
Outdated
| "test:coverage": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --coverage" | ||
| "test": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --testPathIgnorePatterns=tests/integration", | ||
| "test:coverage": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --coverage --testPathIgnorePatterns=tests/integration", | ||
| "compile:tron": "npx hardhat compile --network tron --config tests/artifacts/hardhat.config.cjs", |
There was a problem hiding this comment.
Instead of compiling its smart contract, it would be better to include the artifact directly in the repo like we are doing in wallet-evm:
https://github.com/tetherto/wdk-wallet-evm/blob/main/tests/artifacts/TestToken.json
package.json
Outdated
| "test": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --testPathIgnorePatterns=tests/integration", | ||
| "test:coverage": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --coverage --testPathIgnorePatterns=tests/integration", |
There was a problem hiding this comment.
npm test should run both unit and integration tests:
| "test": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --testPathIgnorePatterns=tests/integration", | |
| "test:coverage": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --coverage --testPathIgnorePatterns=tests/integration", | |
| "test": "cross-env NODE_OPTIONS=--experimental-vm-modules jest", | |
| "test:coverage": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --coverage", |
package.json
Outdated
| "test": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --testPathIgnorePatterns=tests/integration", | ||
| "test:coverage": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --coverage --testPathIgnorePatterns=tests/integration", | ||
| "compile:tron": "npx hardhat compile --network tron --config tests/artifacts/hardhat.config.cjs", | ||
| "test:integration": "npm run compile:tron && cross-env NODE_OPTIONS=--experimental-vm-modules jest tests/integration/module.test.js --testTimeout 120000 --transform '{}' --runInBand", |
There was a problem hiding this comment.
We don't really get any benefit from using the --runInBand option. Actually, it might slow down the execution time of the tests when they are run on our machines since this option makes them run serially on a single thread. It might make sense to enable this flag in the ci/cd of the project though, since it may reduce the running time of the build workflow.
There was a problem hiding this comment.
I'm also not sure about the purpose of the --transform option, i couldn't even find it in the documentation.
package.json
Outdated
| "bare.js", | ||
| "tests/**/*.js" | ||
| "tests/**/*.js", | ||
| "tests/artifacts/hardhat.config.cjs" |
There was a problem hiding this comment.
No need to ignore this file.
tests/artifacts/hardhat.config.cjs
Outdated
There was a problem hiding this comment.
By including the artifact in the repo, hardhat becomes quite useless. We should be able to get rid of it completely, along with the layerzero plugins.
tests/integration/module.test.js
Outdated
| expect(isValid).toBe(true) | ||
| }, TIMEOUT) | ||
|
|
||
| test('should convert a full account to read-only and perform balance, quote and receipt operations', async () => { |
There was a problem hiding this comment.
There's no need for this test.
tests/integration/module.test.js
Outdated
| for (const acc of [acc0, acc1]) { | ||
| expect(acc.keyPair.privateKey).toBeFalsy() | ||
|
|
||
| await expect(acc.sign('Hello, world!')).rejects.toThrow() |
There was a problem hiding this comment.
We should verify that the 'sendTransaction' and the 'transfer' method start throwing as well after disposing the wallet. Also, always assert against a specific error message or pattern instead of just requiring the method to throw:
| await expect(acc.sign('Hello, world!')).rejects.toThrow() | |
| await expect(acc.sign('Hello, world!')).rejects.toThrow('The expected error message...') |
tests/integration/module.test.js
Outdated
| disposableManager.dispose() | ||
|
|
||
| for (const acc of [acc0, acc1]) { | ||
| expect(acc.keyPair.privateKey).toBeFalsy() |
There was a problem hiding this comment.
The private key should be null after disposing the account (see here):
| expect(acc.keyPair.privateKey).toBeFalsy() | |
| expect(acc.keyPair.privateKey).toBe(null) |
tests/integration/module.test.js
Outdated
|
|
||
| const TRANSFER = { | ||
| token: setup.testTokenAddress, | ||
| recipient: await (await walletManager.getAccount(1)).getAddress(), |
There was a problem hiding this comment.
Let's use a different address here too.
tests/helpers/setup.js
Outdated
| /** | ||
| * Blocks until the Tron node is producing blocks. | ||
| * | ||
| * @param {TronWeb} tronWeb |
There was a problem hiding this comment.
If we want to provide doc comments for testing components as well, then we must do it properly. So, either we get rid of them or we provide the missing descriptions for arguments and return values in this method and the methods below.
…fic error message
…actionFee) instead of using the hardcoded constant
1d4fe40 to
4ffb32b
Compare
Description
Motivation and Context
Related Issue
PR fixes the following issue:
Type of change