Addition of assertException and test cases for issue #280 #287

Merged
merged 4 commits into from Feb 2, 2012

Projects

None yet

3 participants

@clexmond

This includes a failing test for the existing expectException implementation as well as a new method, Unit::assertException and related test cases.

@toomuchpete

This is a really nice way to handle unit testing exceptions.

@gwoo
Member

do we need an else here if it is returning already?

The code inside the else statement could probably just be broken out and put at the same level as the if.

@gwoo
Member

Could you explain why a closure is used? Typically the second param is the $result.

The closure is effectively the result here. It would contain code within it that is expected to throw an exception. This is a more flexible and more abstract way of testing for exceptions than having to include try / catch blocks throughout your test cases.

@gwoo
Member

This else if looks like it could be an if.

It could. If that's a general lithium standards preference it can be changed no problem.

@clexmond

@gwoo, anything need to be done that's preventing this from getting pulled?

@gwoo
Member
gwoo commented Jan 30, 2012

I still need to pull this change in locally and test it. I still have a fear that we are changing the convention of writing tests by using the closure as the second parameter. It leads me to call it something other than assertException, since other assert methods generally take the $result as the result as the first or second param. Thoughts?

@clexmond

@gwoo, I can see your concern. Perhaps it would make more sense to update expectException to explicitly cause a passed or failed test. That does mean the excepting code needs to be the final bit of code in a given test case and cannot be followed by any other asserts, but it would be more consistent with the way phpunit and others do it.

@toomuchpete

For what it's worth (probably nothing), I like the way it's handled here far better than any of the proposed alternatives. It allows for testing multiple exception-generating conditions in one test, which I personally find more important than making two things that need to function differently consistent with each other. The expectException method has too many limitations.

That said, there's no reason that assertException() couldn't optionally take a $result from inside of a catch() block for folks who want to do things that way while it also provides this sort of more compact testing for exception-generating conditions if a closure is passed.

That also provides the benefit of allowing assertException() to be used in cases where, for whatever reason, a closure can't generate the exception in quite the same way.

@gwoo gwoo merged commit b6a48ad into UnionOfRAD:master Feb 2, 2012
@gwoo
Member
gwoo commented Feb 2, 2012

Merged.

@toomuchpete I like the ideas.

@clexmond thanks for the effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment