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.

Sign up for GitHub

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

Jump to bottom

Move solidity test suite to truffle beta #846

Merged
merged 1 commit into from
Dec 22, 2018
Merged

Move solidity test suite to truffle beta #846

merged 1 commit into from
Dec 22, 2018

Conversation

coventry
Copy link
Contributor

This probably needs an update to CI, to use the compatible truffle/ganache-cli versions. AFAIK, we'll also need to kick off the right version of ganache-cli independently of truffle, until they're brought into line.

Handling of BigNums vs bigNum is a bit messy. Might make sense to revive assertBigNum to handle all relevant types.

Leaving the interim commits unsquashed in case they're useful for review.

@coventry coventry changed the title Chore/move to truffle beta Move solidity test suite to truffle beta Dec 16, 2018
@coventry
Copy link
Contributor Author

BTW, ganache-cli needs to be run like this:

$ ./node_modules/.bin/ganache-cli -m 'candy maple cake sugar pudding cream honey rich smooth crumble sweet treat'

This should probably be automated in the CI scripts, somewhere.

@coventry coventry force-pushed the chore/move-to-truffle-beta branch from 48b2f45 to 47a2da9 Compare December 17, 2018 16:39
@coventry
Copy link
Contributor Author

@se3000 : Above force-push breaks up the WIP commit, bc9cbf4.

se3000
se3000 reviewed Dec 18, 2018
View reviewed changes
solidity/app/deployer.js Show resolved Hide resolved
solidity/app/wallet.js Show resolved Hide resolved
solidity/truffle.js Show resolved Hide resolved
solidity/truffle.js Outdated Show resolved Hide resolved
solidity/test/support/helpers.js Outdated Show resolved Hide resolved
solidity/test/BasicConsumer_test.js Outdated Show resolved Hide resolved
solidity/test/BasicConsumer_test.js Outdated Show resolved Hide resolved
solidity/test/UpdatableConsumer_test.js Outdated Show resolved Hide resolved
@se3000 se3000 force-pushed the chore/move-to-truffle-beta branch 2 times, most recently from f2918ff to 5ecc4ae Compare December 18, 2018 20:59
@coventry coventry force-pushed the chore/move-to-truffle-beta branch 2 times, most recently from 4ef9add to 1411478 Compare December 19, 2018 22:01
@coventry coventry force-pushed the chore/move-to-truffle-beta branch from b39f410 to 57d0646 Compare December 19, 2018 23:54
@coventry
Copy link
Contributor Author

coventry commented Dec 19, 2018 •
edited
Loading

Hi, @se3000 . This is largely complete, except for this issue in the echo_server test, which seems to relate to a strange behavior of solc-js.

We may want to move away from solc-js for the deployer anyway, if possible.

@coventry coventry force-pushed the chore/move-to-truffle-beta branch 2 times, most recently from 1f3d2cd to 74fd3b4 Compare December 20, 2018 00:26
@coventry
Copy link
Contributor Author

Reverted the CI-initiation commit.

dimroc
dimroc suggested changes Dec 20, 2018
View reviewed changes
Copy link
Contributor

@dimroc dimroc left a comment

Choose a reason for hiding this comment

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

Hey there @coventry, made some requests inline below. Will reach out offline to discuss in detail.

examples/uptime_sla/test/UptimeSLA_test.js Outdated Show resolved Hide resolved
solidity/test/support/helpers.js Show resolved Hide resolved
solidity/test/Coordinator_test.js Show resolved Hide resolved
examples/uptime_sla/test/UptimeSLA_test.js Outdated Show resolved Hide resolved
solidity/test/support/helpers.js Outdated Show resolved Hide resolved
solidity/test/BasicConsumer_test.js Outdated Show resolved Hide resolved
@dimroc
Copy link
Contributor

dimroc commented Dec 20, 2018 •
edited
Loading

Regarding my previous review, a worthy high level goal would be to place the coercion methods inside the helpers so it's centralized.

@coventry
Copy link
Contributor Author

@dimroc, github is not providing me a means to respond directly to your comments because they pertain to outdated commits, so I'll do so here. I marked all your comments as resolved as I responded to them to keep track, but there are still a loose few things in them:

  • Regarding [centralizing conversion of BN transfer amounts](@dimroc, github is not currently providing me a means to respond directly to your comments, so I'll do so here.), see c4985688.

  • Regarding whitespace changes, easy to remove, if you like, but they did make my life a bit easier.

  • Regarding DRYing up the bigNum equality asserts, 517e528bc0

@coventry coventry force-pushed the chore/move-to-truffle-beta branch from c498568 to 7316e9c Compare December 21, 2018 07:15
dimroc
dimroc reviewed Dec 21, 2018
View reviewed changes
solidity/test/support/helpers.js Show resolved Hide resolved
solidity/test/support/helpers.js Show resolved Hide resolved
solidity/test/UpdatableConsumer_test.js Outdated Show resolved Hide resolved
solidity/test/Coordinator_test.js Outdated Show resolved Hide resolved
solidity/truffle.js Show resolved Hide resolved
@coventry
Copy link
Contributor Author

@dimroc, regarding test invocation, (github has removed the option to comment directly, there.) No, you have to run the beta ganache-cli separately and it doesn't use the traditional mnemonic anymore by default, so you have to specify it yourself. I've documented this in 815a8928

@coventry
Copy link
Contributor Author

@dimroc, PTAL

dimroc
dimroc previously approved these changes Dec 21, 2018
View reviewed changes
Copy link
Contributor

@dimroc dimroc left a comment

Choose a reason for hiding this comment

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

I do have some concerns about the new development flow of having to run ganache-cli separate to truffle test. I hope the pros outweigh the cons with truffle 5! But it's the future regardless.

@alexcoventry
Move to truffle beta
54ae6c8 
- Set the compiler version, use docker-based compiler
- Adjustments to work with new truffle/web3: async capture of accounts
- More robust construction of solidity contract search path, for the examples as
  well as the tests
- More systematic check for bytes representation in personalSign
- Better error report for unexpected failure in assertActionThrows
- More direct check that bad signature is generated as expected
- web3.toUtf8 -> helpers.js/toUtf8
- web3.sha3 -> helpers.js/keccack; web3.toWei -> helpers.js/toWei
- Correctly `await` on a number of async functions.
- Wrapper contract link.transfer{,AndCall}, to handle BN amounts
- Move to ganache-cli beta, provide scripts & docs for running it independently
  trufflesuite/ganache#610 (comment)
- Ignore EIP55 capitalization when checking matching addresses
- Update to new version of builder docker image
- Change truffle CI tests to use development ethereum mock
- Make assertBignum handle more "number" types
- Centralize contract transfers to hide conversion of BN's to strings
- Qualify imports of helper functions, in tests which use many of them
@coventry coventry force-pushed the chore/move-to-truffle-beta branch from 815a892 to 54ae6c8 Compare December 21, 2018 22:59
@coventry
Copy link
Contributor Author

Thanks, @dimroc. The need for external ganache ought to be temporary. Once the ganache version used by truffle catches up, we can go back to the old arrangement.

I just squashed to 54ae6c8. Could you please re-approve, once the tests pass?

@coventry coventry merged commit 9adafeb into master Dec 22, 2018
@se3000 se3000 deleted the chore/move-to-truffle-beta branch February 5, 2019 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@se3000 se3000 se3000 left review comments

@dimroc dimroc dimroc approved these changes

@thodges-gh thodges-gh Awaiting requested review from thodges-gh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@coventry @dimroc @se3000 @alexcoventry