Make tests run against Geth or Parity nodes

[rstormsf]

I am not sure how you trust testrpc but I'd expect all tests pass in geth/parity dev chains. For now, it only works if ran against testrpc.
There are 86 failures.
1 that happens a lot is https://github.com/OpenZeppelin/zeppelin-solidity/blob/a344d42a00f47c82cb6ff1abbf39841114c362f6/test/helpers/expectThrow.js - doesn't work with the parity/geth.

[frangio]

Thank you for reporting this. We will have to look into it.

[rstormsf]

here is report https://rstormsf.github.io/zeppelin-test-report/mochawesome.html

[rstormsf]

I believe it also has a lot to do with:
it's only supported by testrpc I belive to quickly change block numbers, time, etc
https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/test/helpers/increaseTime.js

[rstormsf]

Here is my suggestion:
How about instead of relying on

  web3.currentProvider.sendAsync({
        jsonrpc: '2.0',
        method: 'evm_mine',
        id: id+1,
      }

which doesn't exist in parity/geth, we refactor every single occurence of block.number or now
in methods like:

function getBlockNumber() public constant returns(uint256) {
   return block.number;
}

function getTime() public constant returns(uint64) {
  return now;
}

that would allows us to create MockContracts with 1 additional setter:

uint64 public fakeNow;
uint64 public fakeBlockNumber;

function getTime() public constant returns(uint64) {
 return fakeNow;
}

function setTime(uint64 _now) public {
 fakeNow = _now;
}

function getBlockNumber() public constant returns(uint256) {
return fakeBlockNumber;
}

function setBlockNumber(uint256 _blockNumber) public {
fakeBlockNumber = _blockNumber
}

@frangio @maraoz comments?

[frangio]

It's an interesting proposal. Even if we don't migrate to it just now, I think it would make a nice addition to the library for people who want to run their tests on actual blockchains (which arguably should be everyone). Do you want to make a PR for it?

Do you know how many of the failing tests are due to this?

[frangio]

Accidentally closed the issue, sorry.

I would maybe rename getTime to getBlockTimestamp. And also add preconditions to only allow setting values in the future.

It would be nice to see how it will affect the codebase including the tests. I fear it will be too burdensome to have a new parent class for every contract that makes use of block numbers or timestamps.

[rstormsf]

@frangio thank you for your feedback.
I will arrange a PR when I have time to see if I can fix all test cases. I will re-run the test suite since there is some new code/commits and I will report my results.
I can consider that this approach will be approved, so I can give it a try to refactor all contracts.

Almost all non-helper, non-interface contract already has a relative MockContract. So I don't think it would be a problem.
Agree on getBlockTimeStamp since now = block.timestamp in solidity (alias)

For setting values, sure, setters need to check
assert(_now > getBlockTimestamp())

[rstormsf]

@frangio I found another problem:
testrpc would reject transaction and execution if there is broken condifion. For Example:
require(false)
so, right now, we use something like

await assert.isRejected(contract.MethodThatWillThrow());

so, in geth mode, every transaction goes thru, and mined, whether it throws or not.
so chai-as-promised becomes useless with geth and requires a significant amount of refactoring in order to support geth.

Any ideas what could be done about it?

[frangio]

Hm, I had heard about that. We should define the necessary helper functions to abstract away the details.

Last time I checked, given a mined transaction we can look at the used gas to infer whether it succeeded or failed. Maybe we can also use the trace RPC module...

[rstormsf]

@frangio could you explain a bit more on how to read in geth if transaction was failed?

I was also thinking of something like:

async function shouldThrow(cb /* function */, params /* array */){
  const networkId = web3.version.network;
  let txReceipt;
  if (networkId === '123' || networkId === '321') {
    txReceipt = assert.isRejected(cb(...params));
  } else {
    txReceipt = cb(...params);
  }
  return txReceipt;
}

then in test:

await shouldThrow(someContract.methodName, [0, '0x0039F22efB07A647557C7C5d17854CFD6D489eF3']);

[frangio]

We don't need to pass the method and params separately because method calls return promises. Something like expectThrow except if the promise resolves successfully we have to get the transaction receipt or (if available) the trace to check whether the transaction actually succeeded or not.

Sorry I can't write a proof of concept myself at the moment.

[rstormsf]

Still curious how to trace a tx in geth if it actually failed

[nventuro]

We've also discussed having contracts providing msg info, such as msg.sender, etc., to allow easier integration with metatx engines.

[frangio]

There is a PR in geth that might enable this soon. ethereum/go-ethereum#16834

[frangio]

It is not easy and I don't think it's worth the effort to run our unit test suite against Geth. The test suite executes on the EthereumJS VM and I think it's very reasonable to assume that it is a compliant execution client.