Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jest Matchers for Smart Contract Development #321

Merged
merged 26 commits into from Sep 1, 2020

Conversation

adrianmcli
Copy link
Contributor

@adrianmcli adrianmcli commented Jun 19, 2020

Fixes #275

I have begun work on the Jest matchers based off of the Chai Matchers directory. The rationale for providing Jest matchers is located in #275, but in short:

Jest is the leading testing framework in the Javascript world

Jest popularity chart

Chai Matchers (functionality implemented):

  • calledOnContract
  • bigNumber
  • changeBalance
  • changeBalances
  • emit
  • properAddress
  • properHex
  • properPrivateKey
  • reverted
  • revertedWith

The following examples are all tested and working as of today (2020-06-20).

Usage

If you clone the repo and checkout the branch, you can do the following within the monorepo. It's also the expected usage from the end user eventually:

import { waffleJest } from '@ethereum-waffle/jest';

expect.extend(waffleJest);

BigNumbers

expect(await token.balanceOf(wallet.address)).toEqBN(993);

Emitting Events

const tx = await contract.emitOne();

// simple assertion
await expect(tx).toHaveEmitted(contract, 'One');

// assert with args
await expect(tx).toHaveEmittedWith(contract, "One", [
  BigNumber.from(1),
  "One",
  "0x00cfbbaf7ddb3a1476767101c12a0162e241fbad2a0162e2410cfbbaf7162123",
]);

Called on Contract

// simple assertion
await contract.myMethod();

expect('myMethod').toBeCalledOnContract(contract);

// assert with args
await contract.myMethod(1, 2, 3);

expect('myMethod').toBeCalledOnContractWith(contract, [1, 2, 3]);

Revert

// simple revert assertion
await expect(contract.myRevertingFn()).toBeReverted();

// assert with revert reason string
await expect(contract.myRevertingFn()).toBeRevertedWith('Revert reason string');

Balance Changes

const doTransaction = () => sender.sendTransaction({
  to: receiver.address,
  gasPrice: 0,
  value: 200,
});

await expect(doTransaction).toChangeBalances([sender, receiver], ["-200", 200]);

Misc Utils

// assert proper address
expect('0x28FAA621c3348823D6c6548981a19716bcDc740e').toBeProperAddress();

// assert non-proper private key
expect('0x28FAA621c3348823D6c6548981a19716bcDc740').not.toBeProperPrivateKey();

// assert proper hex
expect('0x702').toBeProperHex(2)

@adrianmcli
Copy link
Contributor Author

adrianmcli commented Jun 19, 2020

NTS: The event emitting matchers are not really robust. Things that need to be fixed/tested:

  • Arbitrarily nested data
  • Asserting multiple events on the same transaction
  • Asserting events from multiple contracts on the same transaction

To be honest, we can kill a bunch of those by creating a matcher where you pass in your transaction hash rather than a promise of the contract mutation.

In either case, I think this is good enough as an alpha-level implementation so I will move on to the final calledOnContract matchers instead.

@adrianmcli adrianmcli marked this pull request as ready for review June 20, 2020 07:29
@adrianmcli
Copy link
Contributor Author

The preliminary implementation of these matchers is done. It is now basically feature-complete compared with the Waffle Chai matchers. Before I started working on the docs, I wanted to make sure that there is really interest in adopting this into the Waffle repo.

There were also a few changes I had to make due to limitations (and just different conventions) with Jest. I will name some of them here:

BigNumber Matchers

When overriding existing matchers in Jest, there is no easy way to get access to the underlying matcher that you are overriding. That means we cannot enhance toEqual with BigNumber comparisons without breaking the functionality of the native toEqual matcher that comes out of the box with Jest.

As a result, I have compromised by adding a BN suffix to each matcher instead: toEqBN, toBeGtBN, toBeLtBN, toBeGteBN, toBeLteBN. It's a little ugly, but hey it's better than nothing. I also haven't gotten around to creating the more wordy aliases either.

Events API

Rather than passing in the Promise of the transaction, you will pass in the transaction object returned by Ethers.js instead. This is because the extension API with Jest isn't very conducive to custom chaining operators. Therefore, the best way to support asserting multiple events per transaction is to just allow people to make assertions on a transaction object instead.

For example:

// chai matchers
await expect(events.emitOne())
  .to.emit(events, 'One')

// jest matchers
const tx = await events.emitOne();
await expect(tx).toHaveEmitted(events, 'One');

Events Strict Number Comparisons

When asserting Event args with the Chai matchers, a BigNumber.from(1) is treated exactly the same as "1" or 1. This is a very understandable convenience feature. But with Jest, it is very handy to use jest-diff to display a rich diff of the two arrays of args (expected vs actual).

If we do this, then we need to be more strict with our comparisons. But the benefit of this is a much more user-friendly rich diff in your Jest test output:

Screen Shot 2020-06-20 at 3 45 59 AM

@adrianmcli adrianmcli mentioned this pull request Jun 20, 2020
@adrianmcli adrianmcli changed the title Jest matchers Jest Matchers for Smart Contract Development Jun 20, 2020
@marekkirejczyk
Copy link
Contributor

Hi @adrianmcli,

Looks like you did a lot of awesome work here! We need to allocate a bit more time to review it.
We will try to start reviewing next week.

Marek

Copy link
Contributor

@sz-piotr sz-piotr left a comment

Choose a reason for hiding this comment

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

I really appreciate the work put into this. I left some minor comments that I think will lead to good improvements overall.

waffle-jest/test/matchers/events.test.ts Show resolved Hide resolved
waffle-jest/test/setupJest.ts Show resolved Hide resolved
export const bigNumberMatchers = {
toEqBN(received: Numberish, value: Numberish) {
const pass = BigNumber.from(received).eq(value);
return pass
Copy link
Contributor

Choose a reason for hiding this comment

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

This pattern can be simplified and condensed into:

return {
  pass,
  message: () => pass
    ? `Expected "${received}" NOT to be greater than ${value}`
    : `Expected "${received}" to be greater than ${value}`
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that's more concise, but I'd argue that it's a lot less readable because you are mixing a conditional into an object where one property should correspond to the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

More than fine for experimental merge

@marekkirejczyk
Copy link
Contributor

Hi @adrianmcli,

Would love to merge it and publish. Would you find time to implement @sz-piotr proposals?

@adrianmcli
Copy link
Contributor Author

Sorry, it's been pretty busy for me lately. I'll try to take a look at it this weekend.

@Mrtenz
Copy link
Contributor

Mrtenz commented Aug 29, 2020

Thank you for making this PR. I've tried this PR in my application (using a local version), and it works great. There's a few minor things:

  • Some matchers like toEqBN and toBeReverted don't work when using await expect().resolves.toEqBN instead of await expect().toEqBN. As far as I know, using resolves for promises is the common way for Jest matchers, so it would be nice to see support for it.
  • toHaveEmittedWith doesn't seem to work. Using toHaveEmitted works fine, but when specifying the expectedArgs, it fails.

I'd love to see this updated and merged, so we can use this in production.

@adrianmcli
Copy link
Contributor Author

Happy to look into updating the PR further. Can I get a response from @sz-piotr for the comments first?

Also, it might be worth merging in an imperfect implementation than to wait for everything to be fully fleshed out. That being said, I am not a maintainer so this is just a suggestion.

@marekkirejczyk
Copy link
Contributor

@Mrtenz: Thanks for feedback!
@adrianmcli: Good point. Let's merge it and publish as experimental and we can fix as we go.
Would you solve conflicts and I can merge & publish tomorrow.

@adrianmcli
Copy link
Contributor Author

@marekkirejczyk sounds good!

@adrianmcli
Copy link
Contributor Author

@marekkirejczyk I have rebased off master to fix the yarn.lock conflict and all tests pass.

"node": ">=10.0"
},
"dependencies": {
"@ethereum-waffle/provider": "^3.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to move that dependency to devDependencies, to do so validation in validateMockProvider could be relaxed.


export function validateMockProvider(
provider: any
): asserts provider is MockProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could remove that assertion, lib could work with any provider that has callHistory (e.g. buidler.dev provider)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, now it broke the build I Guess we could define type

interface ProviderWithHistory {
  callHistory ...
}

and keep the assertion with it instead of MockProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where would I put this?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the same file I guess, it is just for validation purposes to let complier know about the type.
If it becomes problematic, just revert the commit and we can fix in in next version.

@adrianmcli
Copy link
Contributor Author

@marekkirejczyk removing the assertion in validateMockProvider seems to have caused Typescript typecheck to fail:

src/matchers/calledOnContract/calledOnContract.ts:16:10 - error TS2339: Property 'callHistory' does not exist on type 'Provider'.

16   const {callHistory} = contract.provider;
            ~~~~~~~~~~~

src/matchers/calledOnContract/calledOnContract.ts:19:6 - error TS7006: Parameter 'call' implicitly has an 'any' type.

19     (call) =>
        ~~~~

src/matchers/calledOnContract/calledOnContractWith.ts:18:10 - error TS2339: Property 'callHistory' does not exist on type 'Provider'.

18   const {callHistory} = contract.provider;
            ~~~~~~~~~~~

src/matchers/calledOnContract/calledOnContractWith.ts:20:6 - error TS7006: Parameter 'call' implicitly has an 'any' type.

20     (call) => call.address === contract.address && call.data === funCallData
        ~~~~

I'm not sure how to fix this though. Would be great if you could provide some guidance!

@marekkirejczyk marekkirejczyk dismissed sz-piotr’s stale review September 1, 2020 08:21

Re-reviewed, fine for experimental release.

@adrianmcli
Copy link
Contributor Author

adrianmcli commented Sep 1, 2020

I've reverted the commit, but not sure why the build is failing. All the tests inside the waffle-jest folder work.

@marekkirejczyk
Copy link
Contributor

marekkirejczyk commented Sep 1, 2020

I downloaded, run test and they pass. Error seems to be related to CircleCI out of memory and is a bit random.

@marekkirejczyk marekkirejczyk merged commit a8ef989 into TrueFiEng:master Sep 1, 2020
@adrianmcli
Copy link
Contributor Author

awesome, thanks for taking a look at this @marekkirejczyk

@marekkirejczyk
Copy link
Contributor

Released in @3.1.0

@adrianmcli
Copy link
Contributor Author

awesome, thanks @marekkirejczyk

@rryter
Copy link

rryter commented Nov 1, 2020

Hi there.

Thanks a lot for this, works pretty nicely. However my IDE (vs-code) does not understand it:

Screenshot 2020-11-01 at 20 34 17 How do I make vscode recognize the matchers? I tried everything I can think of by now. The only thing that works is to copy and paste the typings file into my project.

Any help is greatly appreciated.

@Mrtenz
Copy link
Contributor

Mrtenz commented Nov 2, 2020

@rryter I was running into the same issue with IntelliJ IDEA. I solved it by adding types.d.ts to files in tsconfig.json:

{
  "files": [
    "node_modules/@ethereum-waffle/jest/src/types.d.ts"
  ]
}

@adrianmcli
Copy link
Contributor Author

We should probably add this to the README. Or alternatively, I wonder how we can make the package include the types automatically? @marekkirejczyk

@rryter
Copy link

rryter commented Nov 2, 2020

@rryter I was running into the same issue with IntelliJ IDEA. I solved it by adding types.d.ts to files in tsconfig.json:

{
  "files": [
    "node_modules/@ethereum-waffle/jest/src/types.d.ts"
  ]
}

I tried that, unfortunately it does not change anything. So maybe I have a "does not work on my machine" problem.

This is the "affected" repo: https://github.com/rryter/solidity-playground/blob/main/test/ERC734KeyManager.test.ts#L78

Does anybody else use vs-code and could have a quick look? I'll keep digging.

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.

Jest support
5 participants