Skip to content

Conversation

@pf92
Copy link

@pf92 pf92 commented Sep 20, 2019

This pull request enables deep equality checks for arrays, objects, etc. that contain big numbers.

E.g., now the following check works:
const array1 = [new BN(1), '2', new BN(-10)];
const array2 = [new BN(1), '2', '-10'];
expect(array1).to.have.a.bignumber.and.to.deep.equal(array2);

If bignumber and deep are present in the chain, the assertion will check whether actual deeply equals expected.

@nventuro
Copy link

nventuro commented Oct 7, 2019

Hello @pf92, sorry for taking so long to review this! I was out of office, and the notification for this PR must've slipped under my radar.

I did a cursory review and everything seems okay, but what would be really neat would be to address this in a way so that #5 would be fixed. Having members allows for interesting assertions when coupled with have and include, and is generally more flexible.

The code shouldn't be too different from what you've already written, do you think you could explore taking that approach instead? Thanks!

@nventuro
Copy link

nventuro commented Oct 8, 2019

After thinking about this some, perhaps a full-fledged members et al implementation is unnecessary, and something to simply check for BNs in arrays is enough. That said, I'm not sure if using deep is the best way to achieve this, since it also includes functionality for testing deep equality of objects, nested arrays, etc.

The issue with this is that we don't just test for BN objects, but also convert other types to them (namely strings), so when running a comparison on e.g. [ BN(3), '3', 'value' ], instead of getting a true deep equality, we'll get an error due to 'value' failing to be converted to a BN.

A simpler implementation with a custom flag (e.g. array), that assumes unidimensional arrays where all elements are either BNs or BN-convertible seems to fit our needs much better.

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.

2 participants