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

Add dontExpectEvent.inTransaction to test negative cases #98

Closed
stephen101 opened this issue Nov 18, 2019 · 14 comments
Closed

Add dontExpectEvent.inTransaction to test negative cases #98

stephen101 opened this issue Nov 18, 2019 · 14 comments

Comments

@stephen101
Copy link

I would like to assert that certain events don't get emitted under given circumstances.

Suggested syntax:
await dontExpectEvent.inTransaction(truffleReceipt, 'Foo');

@frangio
Copy link
Contributor

frangio commented Nov 19, 2019

Thanks for the suggestion @stephen101! What do you think about the following syntax, so that we keep this helper in the expectEvent module:

await expectEvent.missing.inTransaction(receipt, 'Foo');

Should this helper also have the option to match the arguments of the event?

@nventuro
Copy link
Contributor

I think expectEvent.not.inTransaction also works (and is consistent with other assertion libraries).

@alejoamiras
Copy link
Contributor

I'm in need of this to test negative cases too 😊. Is anyone on it?

@frangio
Copy link
Contributor

frangio commented Dec 2, 2019

No one is working on this yet. @alejoamiras do you want to submit a pull request?

@stephen101
Copy link
Author

I like @nventuro's suggestion of expectEvent.not.inTransaction

@alejoamiras
Copy link
Contributor

Would something like:

function notInTransaction (txHash, emitter, eventName) {
  expect(() => inTransaction(txHash, emitter, eventName)).to.throw(`No '${eventName}' events found`);
}

Be too nasty ? It would re-use almost all the code. If not, a notInLogs() function could be developed and applied.
On the other hand, the expectEvent.not syntax could be applied to expectEvent.not.inTransaction, expectEvent.not.inConstruction, but would be weird if applied to expectEvent.not.inLogs if expectEvent.inLogs is being deprecated.

@frangio
Copy link
Contributor

frangio commented Dec 9, 2019

Heh, that implementation definitely works but it's not ideal.

An important detail that hasn't been mentioned is whether expectEvent.not.inTransaction supports checking for event arguments. I don't think it should, because the behavior wouldn't be obvious.

If everyone agrees with that, then the helper would only check that no events with the given name are present. The implementation of that function is so simple that I wouldn't worry about reusing all of the code of expectEvent.inTransaction, since most of it deals with checking event arguments.

@alejoamiras
Copy link
Contributor

alejoamiras commented Dec 9, 2019

I thought it was an awful implementation too 😅. I second that expectEvent.not.inTransaction not to check eventArgs since it would be pretty unintuitive.
I'd still like your input in the expectEvent.not.inLogs & dontExpectEvent syntax issue. Maybe having expectEvent.not.inLogs its not that big of a deal.

@frangio
Copy link
Contributor

frangio commented Dec 10, 2019

I agree with your initial comment that we shouldn't implement the negation of inLogs. For deprecated features we should only do bugfixes.

If we add the .not thing it wouldn't be automatically available for all the other functions. "not" would be an object and we would only put in it the functions that we want to provide. It wouldn't be an automated kind of not like in Chai's assertions, just an ad-hoc thing for inTransaction, until we see it would be useful for other functions as well.

@alejoamiras
Copy link
Contributor

Yeah, I didnt thought we were going for the Chai like assertion of not. I've got a mini thing worked. would like ur input.
Wanted to submit a draft PR, or just to push the branch so I could get some input. I did a expectEvent.not as default, like you guys did expectEvent but it looks awful. So, idk really how you guys want to handle it.
Didn't have the permissions to create new branch / commit / etc. Maybe tomorrow I'll be able to dedicate an hour in my working day to end up the details of the branch, if you are able to define them a bit.

@frangio
Copy link
Contributor

frangio commented Dec 10, 2019

You need to fork this repo and push your branch to your fork. Just submit a draft PR and we can go over the details.

@alejoamiras
Copy link
Contributor

Created draft PR: #104

@stephen101
Copy link
Author

Heh, that implementation definitely works but it's not ideal.

An important detail that hasn't been mentioned is whether expectEvent.not.inTransaction supports checking for event arguments. I don't think it should, because the behavior wouldn't be obvious.

If everyone agrees with that, then the helper would only check that no events with the given name are present. The implementation of that function is so simple that I wouldn't worry about reusing all of the code of expectEvent.inTransaction, since most of it deals with checking event arguments.

Agree it shouldn't check arguments. Doesn't make sense to check arguments because you are expecting an event not to happen.

@nventuro
Copy link
Contributor

Closed via #104.

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

No branches or pull requests

5 participants