Replace all asserts with chai.should #1183
Conversation
| @@ -3,6 +3,12 @@ const { ethGetBalance } = require('./helpers/web3'); | |||
|
|
|||
| const LimitBalanceMock = artifacts.require('LimitBalanceMock'); | |||
|
|
|||
| const BigNumber = web3.BigNumber; | |||
There was a problem hiding this comment.
I don't like doing this pattern if we never use BigNumber by itself again. can we use
require('chai')
.use(require('chai-bignumber')(web3.BigNumber))
.should();
There was a problem hiding this comment.
Some test files do use it though (but I'd have to check and see if it's necessary). Maybe we could add some sort of global helper everyone requires, that initializes chai and exposes both bignumber and should.
There was a problem hiding this comment.
actually yeah I was wondering about that; just looked it up and we can do this chaijs/chai#868 (comment) to auto-register should, but we'd have to add a custom one to do bignumber.
There was a problem hiding this comment.
I think just having that one require (and making our setup not so magic) is fine for now.
There was a problem hiding this comment.
I'll add that once this is merged then, to keep the PRs tidy.
There was a problem hiding this comment.
Thank you for taking on this gargantuan task!
Is there a special reason you're using .eq instead of .equal? I feel like this goes against the general rule against abbreviations. A large part of this PR consists of changing this, and I feel quite strongly that equal is better.
Everything else looks good.
test/ownership/HasNoTokens.test.js
Outdated
| assert.equal(ownerFinalBalance - ownerStartBalance, 10); | ||
|
|
||
| finalBalance.should.be.bignumber.equal(0); | ||
| (ownerFinalBalance.sub(ownerStartBalance)).should.be.bignumber.equal(10); |
There was a problem hiding this comment.
Unnecessary parentheses here.
| const profit1 = await ethGetBalance(payee1) - initAmount1; | ||
| assert(Math.abs(profit1 - web3.toWei(0.20, 'ether')) < 1e16); | ||
| const profit1 = (await ethGetBalance(payee1)).sub(initAmount1); | ||
| profit1.sub(web3.toWei(0.20, 'ether')).abs().should.be.bignumber.lt(1e16); |
There was a problem hiding this comment.
This is such a weird test.
There was a problem hiding this comment.
Yup, we need to revise all tests at some point, some have poor coverage/are a bit wonky.
| @@ -417,7 +423,7 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { | |||
| await this.token.increaseApproval(spender, amount, { from: owner }); | |||
|
|
|||
| const allowance = await this.token.allowance(owner, spender); | |||
| assert.equal(allowance, amount + 1); | |||
| allowance.should.be.bignumber.eq(amount + 1); | |||
There was a problem hiding this comment.
Is amount here a BigNumber instance? We should be using BigNumber addition.
There was a problem hiding this comment.
It's not, though it probably should be. Spawned #1189
| @@ -453,7 +459,7 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { | |||
| await this.token.increaseApproval(spender, amount, { from: owner }); | |||
|
|
|||
| const allowance = await this.token.allowance(owner, spender); | |||
| assert.equal(allowance, amount + 1); | |||
| allowance.should.be.bignumber.eq(amount + 1); | |||
Fixes part of #1091.