eclipse-vertx / vertx-junit5

Testing Vert.x applications with JUnit 5
Apache License 2.0
42 stars 30 forks source link

Deprecate succeeding() and failing(), add failingThenComplete() #80

Closed julianladisch closed 4 years ago

julianladisch commented 4 years ago

Using the parameter-less method VertxTestContext#succeeding() is almost always unintended and should be replaced by

The same for failing().

Deprecating for removal the methods succeeding() and failing() improves VertxTestContext's ergonomics because people are no longer tempted to use them. The replacement either fixes an unintended bug in people's unit test code or is more readable because it explicitly states the intention.

Example for unintended succeeding() usage that is not executed: https://github.com/reactiverse/reactiverse-junit5-extensions/pull/3/files

The new method failingThenComplete() is a short-cut for failing(exception -> vertxTestContext.completeNow()) and parallels completing() that is an existing short-cut for succeeding(value -> vertxTestContext.completeNow()).

Note that vertx-unit's TestContext#asyncAssertSuccess() and TestContext#asyncAssertFailure() are different because they complete the test context by design. People switching from vertx-unit to vertx-junit5 will not fall into this trap if we remove succeeding() and failing().

Fixes #77.

jponge commented 4 years ago

Hi @julianladisch, this makes sense indeed.

However, we cannot simply remove methods like this. We should mark them as deprecated, then keep them until at least Vert.x 4.1 / 4.2.

/cc @vietj @slinkydeveloper

julianladisch commented 4 years ago

I've deprecated succeeding() and failing() instead of removing them.

jponge commented 4 years ago

Ok, I'll merge

jponge commented 4 years ago

Thanks @julianladisch