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

Draft test suite #8

Merged
merged 1 commit into from
Dec 15, 2016
Merged

Draft test suite #8

merged 1 commit into from
Dec 15, 2016

Conversation

cgewecke
Copy link
Collaborator

@cgewecke cgewecke commented Dec 14, 2016

This PR implements some of what is described in #7. It can be merged into master independently of anything else and run with npm test. All tests will fail. Currently open PR's address some of these failures. Merging PRs #3, #4, #5, #9, #10, #11 should cause all tests to pass. (Code uses 2 space indentation for JS and 4 spaces for solidity (per their guidelines) but otherwise has no style.)

Core changes:

  • Adds ~30 unit tests - half of these are conventional logic tests and the other half are just contracts from the zeppelin-solidity project. This is really throwing a kitchen sink at solcover and seeing what breaks rather than sitting down and carefully thinking about how things should be, but it kind of works as a way of finding problems. Zeppelin also recently opened an issue to add coverage to their code base.
  • Moves .sol files into their own folder and sub-folders
  • Adds utilities to load .sol files into the suite and handle error reporting from a single point
  • Integrates ifStatements.js into this framework.
  • Updates package.json to launch testing with npm test

@cgewecke cgewecke mentioned this pull request Dec 14, 2016
@area
Copy link
Member

area commented Dec 15, 2016

Applause

Wow, this is brilliant. The reason I had been dragging my feet on the other PRs was that I wanted to get tests set up to confirm they were fixing broken behaviour.

The reason you've found some weird oversights is because I threw our contracts through solcover, and only added new features until they worked, so I can't fault you for a similar approach with using the zeppelin contracts!

I'm torn about whether I'd prefer to have those contracts as a (pinned to a specific commit?) git submodule, rather than adding all their code to this repo. I could go either way. Short term, at least, I don't think I mind at all. I've just seen what comes out the other side when all these PRs are merged locally, and it looks good to me. So I'm going to merge all of these!

@area area merged commit fe42d14 into JoinColony:master Dec 15, 2016
@cgewecke
Copy link
Collaborator Author

LOL capt picard! Thanks @area, that's really sweet of you.

I agree about the zeppelin tests - they shouldn't be in there. Maybe they're ok as a stop gap while instrumentation logic is being written in lieu of something more painstaking? It seems like they run most of what's possible to write in Solidity. In the end, when projects start actively integrating solcover into their builds the bug reports will come flowing in on their own and real tests will write themselves. 😇

For what it's worth I also ran some large gnosis contracts through it and the only compilation issues apart from the empty bodied function were bugs from solidity-parser or code that solc just can't compile. It really validates all the work you did on this. And thanks so much for publishing a solution to this problem - it's incredibly valuable as an audit tool.

I'm thinking about doing some small things to the runCovered script over the weekend unless you're actively working on it. If not you'll probably get another Issue / Draft PR from me on Monday (Tuesday GMT).

@cgewecke cgewecke deleted the draft-tests branch December 15, 2016 19:56
@cgewecke cgewecke mentioned this pull request Dec 15, 2016
area pushed a commit that referenced this pull request Jan 6, 2017
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.

2 participants