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

feat: event-not-emitted support #121

Merged
merged 6 commits into from May 8, 2020

Conversation

AuHau
Copy link
Contributor

@AuHau AuHau commented Apr 27, 2020

Support for expectEvent.notEmitted().

Question: Should there be support for argument specification? If so, how should be handled empty args? Should it search for an empty args event or just generally assert if an event was not-emitted?

Closes #120

I will work on changelog/docs after finalizing API/behavior.

@frangio
Copy link
Contributor

frangio commented May 4, 2020

@AuHau I think you unintentionally commited style changes. Can you look into that and reverse it? And for the new code please use semicolons following the style of the rest of the project.

@AuHau
Copy link
Contributor Author

AuHau commented May 5, 2020

Damn, you are right. Sorry about that. It is fixed.

Btw. I have to used // eslint-disable-next-line no-unused-expressions if you don't wont me to use that there is way using dirty-chai but that would introduce another dependency, so whatever solution is better for you.

@AuHau
Copy link
Contributor Author

AuHau commented May 6, 2020

I am not sure why the tests fail as I did not really do anything related to reason why they fail. It seems like they are missing @truffle/debug-utils package?

@frangio
Copy link
Contributor

frangio commented May 6, 2020

Yes, there is a bug in Truffle, see trufflesuite/truffle#3024. I changed the package.json in a way I think will fix our CI.

This is looking great! There's a few things missing:

  • Settle naming. It has been settled, we like notEmitted, but we should also move the existing negated helpers (not.inTransaction, etc.) into notEmitted so that they can be used with the same syntax. To make it backwards compatible we should alias notEmitted as not, while deprecating not to remove it in the next breaking release.
  • Add documentation for the new helper in the docs.
  • Add an entry in the changelog.

If there's any of this that you're not sure how to handle please let me know.

@AuHau
Copy link
Contributor Author

AuHau commented May 6, 2020

Sounds good! I will make the changes tomorrow :-)

@AuHau
Copy link
Contributor Author

AuHau commented May 7, 2020

Ok, ready for review :-)

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.

Awesome! Just a few suggestions.

CHANGELOG.md Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api.adoc Outdated Show resolved Hide resolved
src/expectEvent.js Outdated Show resolved Hide resolved
AuHau and others added 2 commits May 8, 2020 08:15
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
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 @AuHau!

@frangio frangio merged commit 9691c8c into OpenZeppelin:master May 8, 2020
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.

expectEvent.not for Truffle receipt
2 participants