Skip to content
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

Contract tests #5

Merged
merged 18 commits into from
Jun 19, 2020
Merged

Contract tests #5

merged 18 commits into from
Jun 19, 2020

Conversation

bbenligiray
Copy link
Member

@bbenligiray bbenligiray commented Jun 16, 2020

Set up Jest and wrote an example test (only JS for now). I have some noob questions. Also recommend improvements where you see them.

Does jest (or ts-jest) automatically build the tests for us in the background at the start of each test?

I'm getting

(node:10702) V8: /home/burak/git/airnode/packages/contracts/node_modules/ganache-core/node_modules/rustbn.js/lib/index.asm.js:2 Linking failure in asm.js: Unexpected stdlib member

when I run the test but everything seems to work fine. Is this error familiar to you?

@bbenligiray bbenligiray self-assigned this Jun 16, 2020
Copy link
Member

@andreogle andreogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by "build the tests in the background", sorry. Build what exactly? There is also a really helpful --watch flag that watches for file changes and then re-runs tests for you. I use this a lot when writing tests. See the scripts in oracle-monitor's package.json for some examples I've used.

I'm not sure about that error either unfortunately. I guess if it's not causing anything to break, then it's probably fine to ignore. It looks like it could be related to many things and is hard to pinpoint. Things like your Node version, the Solidity version, your environment etc. I'll try hop onto this branch shortly and see if I get the same.

packages/contracts/jest.config.js Show resolved Hide resolved
"@types/node": "^14.0.12",
"truffle": "^5.1.30",
"ethers": "^4.0.47",
"ethers": "^5.0.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll probably want to extract common dependencies to the root package.json to avoid having to maintain duplicate entries. That would mean most of the deps here would be moved up. Only things like ganache-core would remain.

It may even make sense to just move all dependencies to the root package.json. I'm not sure yet

Copy link
Member Author

@bbenligiray bbenligiray Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract common dependencies to the root package.json

That sounds good if it's not going to mess with Serverless.

It may even make sense to just move all dependencies to the root package.json

This may cause unused dependencies to float around because it's difficult to tell if they are being used in one of the leaf packages

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should mess with serverless, but we'll have to see. lerna is still new to me.

Ok let's keep specific dependencies in the package's package.json

"lint-staged": "^10.2.9",
"prettier": "^2.0.5",
"truffle": "^5.1.30",
"ts-jest": "^26.1.0",
"typescript": "^3.9.5"
},
"lint-staged": {
"*.{js,ts}": [
"npm run prettify",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add linting and static analysis tools for our solidity contracts.

I know that Chainlink uses solhint. Here is a list I found from a quick google search that looks quite good. I have no idea which tools are still maintained though. They are probably even more tools out there
https://consensys.github.io/smart-contract-best-practices/security_tools/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to put these kinds of tools on the strictest settings and then disable rules I don't like/agree with as needed. This is just my preference though and I know that quite a lot of people get annoyed by having lots of linting errors getting in their way lol.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'll set that up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these tools look really good.

https://github.com/trailofbits/manticore
https://github.com/crytic/slither
https://github.com/crytic/echidna
https://github.com/ConsenSys/mythril
https://github.com/melonproject/oyente
https://github.com/pventuzelo/octopus

I imagine that there's a lot of overlap between them but I guess that running a few couldn't hurt? I'm especially a fan of fuzz testing tools

packages/contracts/tsconfig.json Outdated Show resolved Hide resolved
@bbenligiray
Copy link
Member Author

I'm not sure what you mean by "build the tests in the background", sorry. Build what exactly?

I mean the tests are written in TS, but they have no tsconfig, are not built explicitly and built files are not around. That was a bit confusing to me. I'll check out the stuff you've mentioned and come back.

@andreogle
Copy link
Member

I guess that ts-jest just doesn't output any files since it doesn't really need to. I'm not sure of the details though

@andreogle
Copy link
Member

I get the same error when running the tests 🤷‍♂️

V8: /home/andre/Development/airnode/packages/contracts/node_modules/ganache-core/node_modules/rustbn.js/lib/index.asm.js:2 Linking failure in asm.js: Unexpected stdlib member

@bbenligiray bbenligiray merged commit 38bd6d7 into master Jun 19, 2020
@andreogle andreogle deleted the contract-tests branch June 19, 2020 13:23
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.

None yet

2 participants