Skip to content

Conversation

@benjamincburns
Copy link
Contributor

Updates the test suite to use ganache-cli v6.1.0 instead of ethereumjs-testrpc v6.0.1, and to use truffle v4.1.0 instead of truffle v4.0.1.

All tests pass locally when combined with #799.

Note that this test does not update anything to do with testrpc-sc as I don't control releases of that tool, however @cgewecke is aware of this change.

Moved truffle-config.js to truffle.js as this is the filename expected for the main truffle configuration now. Not having a truffle.js caused me to run into ConsenSys-archive/truffle#838.

Copy link
Contributor

@shrugs shrugs left a comment

Choose a reason for hiding this comment

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

just some nitpicks 👍


- Which version of OpenZeppelin are you using?
- What network are you deploying to? testrpc? Ganache? Ropsten?
- What network are you deploying to? ganache-cli? Ganache? Ropsten?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to just say What network are you deploying to? Ganache? Rinkeby?


before(async function () {
// Advance to the next block to correctly read time in the solidity "now" function interpreted by testrpc
// Advance to the next block to correctly read time in the solidity "now" function interpreted by ganache
Copy link
Contributor

Choose a reason for hiding this comment

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

So we've obviously copy-pasted these test files over and over again 😅 can we just remove these duplicate comments in all of these crowdsale tests? I could also see an argument for the extra verbosity being worth it.

gasPrice: 0x01,
},
testrpc: {
"ganache-cli": {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's change this to just ganache?

host: 'localhost',
port: 8545,
network_id: '*', // eslint-disable-line camelcase
network_id: '*' // eslint-disable-line camelcase
Copy link
Contributor

Choose a reason for hiding this comment

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

was this enforced by our eslint config or your editor? I think our eslint config enforces the trailing multiline comma

@cgewecke
Copy link
Contributor

Hi @shrugs, just leaving a note here re: solidity-coverage since Ben Burns mentioned it. Its testrpc client has been updated to use the newest ganache-core (so should be more stable) with 0.4.12.

Happy to open a PR if that's helpful but there are no associated changes apart from the version number.

@shrugs
Copy link
Contributor

shrugs commented Mar 15, 2018

Thanks for the note, @cgewecke! sounds like solidity-coverage should be fine as-is, then.

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.

4 participants