rkalis / truffle-assertions

🛠 Assertions and utilities for testing Ethereum smart contracts with Truffle unit tests
https://kalis.me/check-events-solidity-smart-contract-test-truffle/
MIT License
154 stars 20 forks source link

Cannot read property 'filter' of undefined #27

Closed tcurdt closed 5 years ago

tcurdt commented 5 years ago

Describe the bug

Maybe a user error but the most simple test fails with an undefined filter error.

Example test code

Bellow is the example code but I guess any contract will do.

const tassert = require('truffle-assertions')
...
contract("Request", async accounts => {
  it("creates fine", async () => {
    let project = await Project.new()
    let request = await Request.new(project.address)
    tassert.eventEmitted(request, 'Foo')
  })
})

Expected behavior

I would expect the assert to fire (or not fire) but not get this TypeError.

 1) Contract: Request
       creates fine with project:
     TypeError: Cannot read property 'filter' of undefined
      at assertEventEmittedFromTxResult (node_modules/truffle-assertions/index.js:61:30)
      at Object.eventEmitted (node_modules/truffle-assertions/index.js:176:5)
      at Context.<anonymous> (test/TestRequest.js:21:13)
      at processTicksAndRejections (internal/process/task_queues.js:89:5)

Environment Information Truffle version: v5.0.14 (core: 5.0.14) Web3 version: v1.0.0-beta.37 truffle-assertions version: 0.9.0

tcurdt commented 5 years ago

I guess I figured it out

let result = await tassert.createTransactionResult(request, request.transactionHash);
tassert.eventEmitted(result, "LogCreated")

but a) it's quite ugly b) I must have missed that in the docs/tutorials

rkalis commented 5 years ago

Hey @tcurdt, the reason for this is that Contract.new() doesn't return a TransactionResult object, like function calls do. Instead it returns the new contract instance, which makes sense, but for this case it's a bit inconvenient. I added createTransactionResult() so it would still be possible to assert events inside constructors. It also works for other use cases, such as #6, but I agree it's not really an ideal situation.

In #1 I sketched an alternative syntax, but it is quite an undertaking to refactor to this, which is why I haven't done work on that yet. Since this would also most likely be backwards incompatible and a bit more complex, I'm a bit hesitant to do it at all, as the current syntax works well enough.

Since you missed it, where would you have expected to see about this in the docs?

tcurdt commented 5 years ago

It already is in the docs but I guess I just missed it because I didn't know it's applicable for my situation. What I think would be great is to throw a more specific error if the TransactionResult is missing. One pointing to createTransactionResult. Makes sense?

rkalis commented 5 years ago

That makes sense. I'll see if I can improve the error message.

rkalis commented 5 years ago

Updated in 0.9.1. Thanks again for opening this issue.