[3.4] Assertion improvements

[LeoColman]

The module kotlintest-assertions has some room for improvements. There are many matchers/assertions that are deprecated and were moved, and many matchers have no documentation and not enough tests.

Breaking change: Remove deprecated methods

This issue aims to mark as error all deprecated methods, forcing users to change their imports to the new ones. The deprecated methods will be moved to a legacy directory, that will be removed

This will be an issue to current users, but will save new and old users of the following situation:

"Which import should I use?"
"Oh, this one is deprecated, then I believe it's the other one"

I've noticed that this happens a lot to new users. The confusion is more of an annoying thing than a big problem, but it's bad for user experience. Marking these deprecations as an Error will provide users a quick way to move to the new versions, and will remove this problem.

Code improvement: All tests, in assertions module itself

Some matchers aren't tested for corner cases. Some also don't have any code coverage. To allow new matchers to be developed, and an improvement to assertion quality assurance, all matchers should be covered by unit tests, guaranteeing their functionality.

To further improve the assertions module, their tests can be moved to its module, and not the global kotlintest-tests module. In the near past, 2 contributors were asked to create tests to their new features, and neither of them knew at first where to place the tests. If possible, moving all tests to be near their test-targets will encourage new contributors to add more tests.

However, I remember @sksamuel commenting that due to module building order, it wouldn't be possible to do this. If it is, it would be better for contributors to understand what to do directly

Documentation improvements: Add KDocs to all matchers

Good documentation in-code will allow users to know what a matcher/assertion does without opening Github and scanning through the documentation there. It improves the user experience and the resilience of matchers, as we'll have the documentation of exactly how it does (our current docs have short sentences. Maybe they're not explicit enough to some users).

Contributing to this

I created the branch feature/assertions-improvements. If you have any contribution related to the above comments, feel free to open a Pull Request to that branch!

[LeoColman]

@sksamuel I know that forcing users to change their packages due to deprecation is very bad, but I don't know if we should keep the deprecated methods.

What do you think about this?

[sksamuel]

I am happy for the deprecated methods to be removed in 3.3

[LeoColman]

And then we do all the changes at once, to avoid users having to change everything everytime

[sksamuel]

Let's not move them all for the sake of it but ones like shouldBe definitely need to be removed now.
And assertion tests can move into the assertions module. It's the core tests (testing the engine itself) that must exist in a separate module.

[LeoColman]

I believe we should remove all the deprecated now, to avoid having to create issues to user in the future again due to deprecation.

My line of thought is "well... If they will have to change imports for this now, it's better to change for the others now than having to change them again in the future"

[sksamuel]

Anything that is deprecated can be removed (unless it was deprecated in 3.2, then it's too early).
Should be one full release cycle where it is deprecated.

[LeoColman]

I agree. If anything else gets moved, deprecate it for now? (Foe instance 2 of the same matchers in different places)

…

On Wed, Dec 5, 2018, 20:24 Stephen Samuel ***@***.*** wrote: Anything that is deprecated can be removed (unless it was deprecated in 3.2, then it's too early). Should be one full release cycle where it is deprecated. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#498 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABgRIw7j4DIQG1VH6GVum1lkCGl8hDmGks5u2Ee1gaJpZM4ZDoed> .

[sksamuel]

Yep.

[sksamuel]

One enhancement I think would be really nice would be the ability to do soft asserts in a closure:

someObj should {
  matcher1
  matcher2
}

For example

name should {
  haveLength(3)
  beDigitsOnly()
  include("!")
}

Would have to think about how to include not's in there.

An alternative could be

name.shouldHaveLength(3).beDigitsOnly().include("!")

[sksamuel]

Another enhancement I think is to deprecate shouldHave, and just keep should and rename all matchers to work with the consistent "should" verb.

[LeoColman]

Closing this. It's starting to get old, and not exactly related to changes we're doing now.

If th need rise in the future, I'll reopen this