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

Use expectEvent to test logs #1232

Closed
nventuro opened this issue Aug 23, 2018 · 5 comments
Closed

Use expectEvent to test logs #1232

nventuro opened this issue Aug 23, 2018 · 5 comments
Labels
good first issue Low hanging fruit for new contributors to get involved! tests Test suite and helpers.
Milestone

Comments

@nventuro
Copy link
Contributor

nventuro commented Aug 23, 2018

Most log tests directly access the log array: we should instead use the expectEvent helper to do that.

Example from the Escrow tests:

const { logs } = await this.escrow.deposit(payee1, { from: owner, value: amount });
expectEvent.inLogs(logs, 'Deposited', { payee: payee1 });

Part of #1091.
Requires #1026.

@nventuro nventuro added good first issue Low hanging fruit for new contributors to get involved! kind:improvement tests Test suite and helpers. labels Aug 23, 2018
@nventuro nventuro added this to the v2.1 milestone Aug 23, 2018
@jbogacz
Copy link
Contributor

jbogacz commented Sep 19, 2018

Hey, apart of main work for this task I faced the case where logged event argument is uint256. At truffle test level this argument is represented by BigNumber, so I tried to verify if argument was logged.

For instance:
const event = expectEvent.inLogs(currentSnapshotId.logs, 'Snapshot', { id: new BigNumber(1) });

Even the values are equal the check fails due the arguments are both struct. Please correct me if I'm missing something but I have feeling that expectEvent.inLogs method should verify equality like:
if (!_.isEqual(e.args[k], v)) {

I have tested it for my case with BigNumber and it works fine.

@nventuro
Copy link
Contributor Author

Hey there @jbogacz! You're absolutely right, expectEvent doesn't work properly with BigNumber values, which is why some of the tests mix both styles (e.g. they use expectEvent for event name, account, etc, but then test the values separetely). We actually have a separate issue for this: #1026, if you can open a PR with your change to the test helper, that'd be amazing!

@jbogacz
Copy link
Contributor

jbogacz commented Sep 19, 2018

Yes Sir! I will do it shortly :)

jbogacz added a commit to jbogacz/openzeppelin-solidity that referenced this issue Sep 24, 2018
nventuro referenced this issue in jbogacz/openzeppelin-solidity Sep 26, 2018
nventuro pushed a commit that referenced this issue Sep 27, 2018
* Add BigNumber support to expectEvent/inLogs (#1026)

* switched direct logs array check to expectEvent method in AllowanceCrowdsale.test.js

* Refactor expectEvent.inLogs function to use simple value number check

* Introduced should.be.bignumber method to compare BigNumber values

* Use expectEvent to test logs (#1232)

* Removed trailing space
@nventuro
Copy link
Contributor Author

Closed via #1343

come-maiz pushed a commit that referenced this issue Oct 21, 2018
* Add BigNumber support to expectEvent/inLogs (#1026)

* switched direct logs array check to expectEvent method in AllowanceCrowdsale.test.js

* Refactor expectEvent.inLogs function to use simple value number check

* Introduced should.be.bignumber method to compare BigNumber values

* Use expectEvent to test logs (#1232)

* Removed trailing space

(cherry picked from commit 536262f)
@come-maiz come-maiz modified the milestones: v2.1, v2.0 Oct 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Low hanging fruit for new contributors to get involved! tests Test suite and helpers.
Projects
None yet
Development

No branches or pull requests

4 participants
@come-maiz @nventuro @jbogacz and others