search

tylercollier / redux-form-test

Shows how to do unit tests and integration tests with Redux-Form
219 stars 26 forks source link

Assertions are being ignored #3

Closed bettiolo closed 8 years ago

bettiolo commented 8 years ago

If I invert the assertion, all the tests are still passing

tylercollier commented 8 years ago

Thanks for pointing this out! I have fixed the problem in master. Rather than using the syntax expect(onSave).to.have.been.called, I now use expect(onSave.callCount).to.equal(1). So, the subject under test was working as expected, but the tests were flawed.

Here's what happened if you're curious. Initially I included the sinon-chai library to show off how it makes it easier to read assertions. Someone mentioned a problem in #2, and so I took it out, thinking that the lines of code currently in the source didn't make use of sinon-chai. But they did. It was just hard to tell. One big problem with chai IMHO is that the assertions (that don't take any arguments) are implemented as javascript properties, aka getters. It's nice because it looks more like english prose. You can read more about this here. That article says:

However there could also be confusion when accessing what you think is a property, which could have unintended consequences. This pattern should be used with great care.

...but doesn't elaborate. What you found was a perfect example of an unintended consequence, aka a problem. Another example would be that misspellings can go unnoticed. So if we had written

expect(onSave).to.have.been.calledADirtyRottenName

the test also would have passed. The javascript engine would have looked up the key for calledADirtyRottenName, which is undefined, noted that nothing in the code uses that value, and moved on. You'd never know that it wasn't actually running an assertion, which is what happened to us.

So, what do we do about this call? (which is in the same integration test file):

expect(firstNameHelpBlock).to.exist

If we rewrote it, it'd be much more verbose:

expect(firstNameHelpBlock).to.not.equal(null)
expect(firstNameHelpBlock).to.not.equal(undefined)

(since chai's exist assertion checks both). I don't actually know a better answer. However, in our case, checking that the return value from enzyme's find() exists actually isn't helpful, as it'll always return a value. Instead, what's more helpful is making sure the number of elements is expected (in that particular case, 1). I was hoping chai-enzyme's exist method would help us, but it doesn't do what we want. Thankfully, checking for a length is built into chai's core, so we can use:

expect(firstNameHelpBlock).to.have.length.of(1)
bettiolo commented 8 years ago

For these reasons and the (complicated) verbose syntax, I prefer assert.ok(onSave.called);

tylercollier commented 7 years ago

A follow-up for anyone interested. Above, I said:

I don't actually know a better answer

I now use dirty-chai to address this problem.

// Without dirty-chai. Could fail without alerting you.
expect(true).to.be.true;

// With dirty-chai. Can't escape actually running the assertion.
expect(true).to.be.true();
bettiolo commented 7 years ago

@tylercollier I think that v4 of Chai is much more strict, doesn't it cover that case?

You are using 3.5: https://github.com/tylercollier/redux-form-test/blob/21d1805d0e37e307d71f94f97f852924453c5a78/package.json#L15

tylercollier commented 7 years ago

You're right. Neat! This is documented here as the first bullet under "New Features". It reads "Throw when non-existent property is read..." and documents the issue.

As it's still a canary release, I won't bother updating for now. But thanks for pointing it out.