-
Notifications
You must be signed in to change notification settings - Fork 162
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
Conversation
NTS: The event emitting matchers are not really robust. Things that need to be fixed/tested:
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 |
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 MatchersWhen 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 As a result, I have compromised by adding a Events APIRather 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 ComparisonsWhen asserting Event args with the Chai matchers, a 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: |
Hi @adrianmcli, Looks like you did a lot of awesome work here! We need to allocate a bit more time to review it. Marek |
There was a problem hiding this 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.
export const bigNumberMatchers = { | ||
toEqBN(received: Numberish, value: Numberish) { | ||
const pass = BigNumber.from(received).eq(value); | ||
return pass |
There was a problem hiding this comment.
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}`
};
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Hi @adrianmcli, Would love to merge it and publish. Would you find time to implement @sz-piotr proposals? |
Sorry, it's been pretty busy for me lately. I'll try to take a look at it this weekend. |
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:
I'd love to see this updated and merged, so we can use this in production. |
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. |
@Mrtenz: Thanks for feedback! |
@marekkirejczyk sounds good! |
6893197
to
9264ec1
Compare
@marekkirejczyk I have rebased off master to fix the |
"node": ">=10.0" | ||
}, | ||
"dependencies": { | ||
"@ethereum-waffle/provider": "^3.0.0", |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@marekkirejczyk removing the assertion in
I'm not sure how to fix this though. Would be great if you could provide some guidance! |
Re-reviewed, fine for experimental release.
ab226a1
to
87ce88f
Compare
I've reverted the commit, but not sure why the build is failing. All the tests inside the waffle-jest folder work. |
I downloaded, run test and they pass. Error seems to be related to CircleCI out of memory and is a bit random. |
awesome, thanks for taking a look at this @marekkirejczyk |
Released in @3.1.0 |
awesome, thanks @marekkirejczyk |
@rryter I was running into the same issue with IntelliJ IDEA. I solved it by adding {
"files": [
"node_modules/@ethereum-waffle/jest/src/types.d.ts"
]
} |
We should probably add this to the README. Or alternatively, I wonder how we can make the package include the types automatically? @marekkirejczyk |
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. |
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
Chai Matchers (functionality implemented):
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:
BigNumbers
Emitting Events
Called on Contract
Revert
Balance Changes
Misc Utils