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

Adds negative check to event emittion #104

Merged
merged 7 commits into from
Dec 30, 2019

Conversation

alejoamiras
Copy link
Contributor

No description provided.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thank you @alejoamiras!

src/expectEvent.js Outdated Show resolved Hide resolved
src/expectEvent.js Outdated Show resolved Hide resolved
@alejoamiras
Copy link
Contributor Author

alejoamiras commented Dec 18, 2019

@frangio are these tests exhaustive enough ? Or should I test all possibilities within context('with arguments'). If so, how exhaustive do we want them to be ? (ShortUint, Int, Boolean, combinations of multiple arguments and so on)

@frangio
Copy link
Contributor

frangio commented Dec 19, 2019

@alejoamiras These are perfect IMO. Thank you so much!

@frangio frangio marked this pull request as ready for review December 19, 2019 18:18
@frangio
Copy link
Contributor

frangio commented Dec 19, 2019

Should we go ahead and merge?

@nventuro
Copy link
Contributor

nventuro commented Dec 19, 2019

Great work @alejoamiras, thanks!

Would you mind adding an entry on the changelog and documentation for this addition?

Note that you forked before we added the documentation site proper: you'll want to rebase or merge master into your fork or the docs directory won't be present on your branch.

Thanks!

@alejoamiras
Copy link
Contributor Author

I've updated docs & changelog.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thanks! I've tweaked the description for the function a little bit because I felt it wasn't clear enough.

Great work!

@frangio frangio merged commit 81081bf into OpenZeppelin:master Dec 30, 2019
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.

3 participants