Test suite unhandled promise rejection

[GregTheGreek]

Describe the bug

There is an unhandled promise rejection within the test sutie for intToBytes that does not cause the CI to fail.

Expected behavior

The CI should fail, and the test should probably also fail.

Steps to Reproduce

1. yarn install
2. yarn test

Screenshots

  intToBytes
    ✓ should correctly serialize number to bytes length 1
    ✓ should correctly serialize BN to bytes length 1
    ✓ should correctly serialize number to bytes length 2
    ✓ should correctly serialize BN to bytes length 2
    ✓ should correctly serialize number to bytes length 3
    ✓ should correctly serialize BN to bytes length 3
    ✓ should correctly serialize number to bytes length 4
    ✓ should correctly serialize BN to bytes length 4
    ✓ should correctly serialize number to bytes length 8
    ✓ should correctly serialize BN to bytes length 8
    ✓ should correctly serialize number to bytes length 32
    ✓ should correctly serialize BN to bytes length 32
    ✓ should correctly serialize number to bytes length 48
    ✓ should correctly serialize BN to bytes length 48
(node:67078) UnhandledPromiseRejectionWarning: Error: network does support ENS (operation="ENS", network="unknown", version=4.0.27)
    at Object.throwError (/Users/gregmarkou/Jobs/code/github.com/ChainSafe/lodestar/node_modules/ethers/errors.js:76:17)
    at /Users/gregmarkou/Jobs/code/github.com/ChainSafe/lodestar/node_modules/ethers/providers/base-provider.js:1007:24
    at process._tickCallback (internal/process/next_tick.js:68:7)
(node:67078) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:67078) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Desktop (please complete the following information):

• OS: OSX
• Branch: Master

[mpetrunic]

That error is thrown from ethers.js when contract isn't found, I've looked into when building cli, it's actually ethers.js error which cannot be easily caught in our code other than hack like this:

process.on('unhandledRejection', error => {});

ethers-io/ethers.js#189

[GregTheGreek]

But why is it being thrown in the middle of the test suite?

[mpetrunic]

I'm not 100% on this, but I think mocha executes before of each describe before running it so my guess is that it happens in some before hook and since it's async it get's thrown in the middle of test execution. I might be wrong, that's my guess.

[GregTheGreek]

Do you know the specific test that's causing it? Perhaps we can make it async if possible?

[mpetrunic]

Comment out ethers.test.ts and error will be gone. It happens there because contract address is TBD:
https://github.com/ChainSafe/lodestar/blob/master/test/unit/eth1/ethers.test.ts#L16

[mpetrunic]

I can easily fix tests by using sinon to stub Eth1Notifier constructor, but the error is quite annoying since it cannot be catched (it's thrown inside ethers.js contract constructor, so I think we should add checker is contract exists before initiating ether.js contract constructor

[GregTheGreek]

We can hardcode the contract Address if we wanted to.

Assume its always deployed by the same mnemonic at nonce 0 (or 1? not sure what default is) then the address will always be the same

[mpetrunic]

No need, since those are unit tests and they don't actually listens on smart contract events (deposit contract is not even deployed). We just need to skip constructor execution in Eth1Notifier. As for integration tests, I've already setup contract deployment and it uses deployed address.

But if someone is going to use cli and input wrong contract address or connects to wrong network, they will receive this misleading error unless we check for contract before ethers.js.

[GregTheGreek]

hmmm i see what you're saying - makes sense. I'm thinking for the tests though, if we need to do tests we can standardize that contract address

[mpetrunic]

if we need to do tests we can standardize that contract address

We already solved it like this:
https://github.com/ChainSafe/lodestar/blob/master/test/unit/eth1/deploy.ts#L25

[GregTheGreek]

mmm yeah that always works too