Make matchers more consistent

[helmbold]

should be... vs shouldBe should be unified. And I would prefer to have a consistent syntax in usual function call style OR DSL style (without dots and parenthesis), but not both. See my post.

[helmbold]

The expect.kt library is not bad, although I would prefer a single style and having having a separate negation (not) would be nice. Another thing that I would change is the and , since it adds some clutter (though readable) and looks bad with line breaks. But I think this lib could be an inspiration.

I like it, when the tested value is in the front of the line.

What do you think of a syntax like this?

false.should.not.beTrue

"".should.beEmpty

"james".should
    .haveLength(5)
    .startWith("j")
    .contain("me")

[sksamuel]

I like the chaining aspect. I like the fact with dot syntax you can do more, because its less ambiguous to the compiler. I also like the fact that expect.kt offers several styles, even though you don't like this (doesn't really matter if we have .should and .is or whatever?)

[sksamuel]

I added a reply in the groups, that I sent ages ago but didn't seem to appear @helmbold

[sksamuel]

I've added an attempt to make the DSL more readable. There's still a bit more left to do, but wanted to see what you think first.

https://github.com/kotlintest/kotlintest/blob/issue138/src/test/kotlin/io/kotlintest/matchers/MatcherDsl.kt

[helmbold]

I like the DSL, it is clean and readable. Some thoughts:

• 1 shouldBe beGreaterThan(0) and beLessThan(2) I think the first Be (in shouldBe) shouldn't be there?
• I wonder, whether the and is worth it, although it looks very nice and readable. Have you considered how it looks/works with line breaks?
• I think it would be useful to have a separate not. But in both cases (shouldNot and should not) it could be ambiguous, since it could be confusing what parts are actually negated. That would be better if we had two variants of each matcher (e. g. contain and notContain).
• How would you express a parameterless matcher: beTrue or beTrue()?

"hello world" shouldNot startWith("hello") and endWith("world") and include("!")
"hello world" shouldNot
    startWith("hello") and 
    endWith("world") and 
    include("!")

"hello world".shouldNot.startWith("hello").endWith("world").include("!")
"hello world".shouldNot
    .startWith("hello")
    .endWith("world")
    .include("!")

[sksamuel]

1. Yes, duplicate 'be' is just a mistake
2. Works with line breaks as such

    "hello world" should
        startWith("hello") and
        endWith("world") and
        include("")

3. shouldNot is better because should not won't parse without something else in there too. We can provide inverted matchers like notContain as well, so then the user can choose whatever they want.
4. I don't think it really matters, you can choose.

[sksamuel]

There is an issue I'm trying to work around. In order to support and and or we need to return some kind of builder object from the should function, since in Kotlin the compiler will not consider a look ahead when parsing infix functions. Which means the should function cannot be the function that invokes the matcher (it has to return a matcher accumulator of some sort and can't do both things). I work around this by having the should function add the accumulator to the list of matchers in the spec class, which it then can invoke after the test function itself returns. However, this won't work with inspectors, as they expect the matchers to throw exceptions. One solution would be to have any code in an inspector wrap itself around any matchers added, kind of like the interceptors.

[helmbold]

I wouldn't use the spec class for the matcher DSL. This would introduce a strange dependency in the code. I would design the matchers so that we could release them separately (what could be a good idea for users of other test frameworks). I'd prefer a different syntax if it is not possible to achieve the aspired solution.

[helmbold]

And, by the way, I wouldn't introduce or, since it would need to have some grouping then. If such a logic is really needed in a test, it could be expressed with usual Kotlin constructs and only the result could be tested with matchers.

[helmbold]

Another nice assertions lib (for JavaScript) is Chai

[helmbold]

I think shouldBe should test for idenentity:

infix fun <T> T.shouldBe(any: Any?): Unit = shouldEqual(any)
infix fun <T> T.shouldEqual(any: Any?): Unit 

[sksamuel]

Identity??

…

On 18 Jan 2017 20:20, "Christian Helmbold" ***@***.***> wrote: I think shouldBe should test for idenentity: infix fun <T> T.shouldBe(any: Any?): Unit = shouldEqual(any)infix fun <T> T.shouldEqual(any: Any?): Unit — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtZGgMH-hRoMCN23SJ7Tq_yNkWi8eW3ks5rTnPvgaJpZM4Lk__m> .

[helmbold]

If we have shouldEqual we should use shouldBe to check referential equality (or identity): a === b

[sksamuel]

They're just synonyms. We can't keep changing semantics on people.

[sksamuel]

Just looked at Chai and it looks similar to the others. Its what you'd write if you couldn't use infix methods, as you can't in Javascript. I mean it looks fine, but it offers 3 different styles and you said you prefer having only one style - which one of those do you like the most?

I am all for trying to tidy things up, but I do think a lot of the discussion here is just "it's not the way I'd have written it" which is not a valid reason for making people change all the tests they've written using the matchers from KotlinTest 1.x.

I think that unless there's a good reason to change things (and there might well be lots of good reasons and I'm totally in favour of us debating it) that we should keep it the same. I actually like the scalatest style and I'm sure others do, and others won't including yourself.

[helmbold]

Maybe some things are just a matter of taste, but we should try to be as consistent as possible - and that was my original intention (e. g. should be... vs shouldBe). Now is the best time to change such things, since I expect that the adoption would increase significantly after 2.0 release (not overnight, of course).

[sksamuel]

I agree its best to lock it down now, and I also agree that should be vs shouldBe is noise. That can go.

[helmbold]

I thought about parameterless matchers like isTrue and prefer functions over properties here, because it is easy to remember that we're always using function and because it has a side effect (throwing an exception).
Example: isTrue() instead of isTrue

[sksamuel]

Agreed

[sksamuel]

So looking over our matchers again and I'm ok with it. I'm personally not that bothered about should and shouldBe being aliases - it means you can write whichever sounds more grammatically correct.

I've removed the 'be' keyword, and the keyword interface itself. That's noise which we don't need. This means all matchers must simply be a function which returns a Matcher<T> instance, and we drop any ideas of doing fancy this should have length 10 in favour of just this should haveLength(10).

I'm in favour of closing this now.

[helmbold]

We have still this inconsistency with shouldBe and should be.... You can write things like this: "x" shouldBe beEmpty<String>(), whereas x" should beEmpty<String>() is not possible.

I think we should only provide should and shouldNot.

[sksamuel]

Yes you can, but you'd have to be a fool to do that. We can't stop people being fools and we shouldn't try.

[helmbold]

How would you test for an empty string? As mentioned above "x" should beEmpty<String>() doesn't work, but that would be the obvious way (despite the possibly unnecessary type parameter).

[sksamuel]

Could add `str shouldBe emptyString()`

…

On 18 Mar 2017 13:35, "Christian Helmbold" ***@***.***> wrote: How would you test for an empty string? As mentioned above "x" should beEmpty<String>() doesn't work, but that would be the obvious way (despite the possible unnecessary type parameter). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtZGiUZAhhZyu7valnsBHAGequzztmQks5rm92pgaJpZM4Lk__m> .

[helmbold]

I wouldn't add emptyString, since this is not really better than beEmpty<String>. I thought of using a star projection in fun <T> beEmpty(): Matcher<Collection<T>>, but I didn't find a way to do it. In principle Collection<*> would be sufficient.

But my main point is, that "x" should beEmpty<String>() doesn't compile, and that we have the confusing alternatives of should be... and shouldBe.

[sksamuel]

I know why you were bringing it up :)

…

On 18 Mar 2017 21:40, "Christian Helmbold" ***@***.***> wrote: I wouldn't add emptyString, since this is not really better than beEmpty<String>. I thought of using a star projection in fun <T> beEmpty(): Matcher<Collection<T>>, but I didn't find a way to do it. In principle Collection<*> would be sufficient. But my main point is, that "x" should beEmpty<String>() doesn't compile, and that we have the confusing alternatives of should be... and shouldBe. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtZGlkby8b4BYX20jROyz9XlhHqz0jGks5rnE9kgaJpZM4Lk__m> .

[sksamuel]

What's wrong with adding an emptyString matcher. Although you could use the collection matcher, there's no problem with having one just for string that means it reads easier for someone reading the code?

[helmbold]

Adding emptyString as a special case would be a little inconsistency and would affect learnability. Why not emptyList then ... ?

[sksamuel]

It's not a special case anymore than String itself is a special case. I'd be happy to have emptyList too. What's wrong with extra options if that means that the tests read clearer? What's the disadvantage to adding them - some kind of purity that means nothing?

[helmbold]

I want to avoid that someone wonders about a missing method like emptyThing. If we just provide the generic method, empty<Thing>() is obvious. Actually I'd prefer empty(), since it is not important whether it is a String, a List or some other type. And using a type parameter separates the important empty from the less important type.

However that is not the point I want to make. I could live with emptyString(), emptyList() and so on. What is important, is that we have a way to make "x" should beEmpty<String>() compile and that we remove shouldBe.

[sksamuel]

Ok, if we remove shouldBe then what do we do about 3 shouldBe lessThan(4) ?

3 should beLessThan(4) which is less readable imho.

I don't know why it's important to not have should and shouldBe. There's many different ways of doing things in Java, Kotlin and Scala. That's part of a powerful language.

I do,

option.foreach(builder.add)

You do

option.foreach { x => builder.add(x) }

Your friend does

option match {
  case Some(x) => builder.add(x)
  case _ =>
}

The great thing about should and shouldBe is that we can let the user choose which one is more readable for their test. When it comes to custom matchers, we should probably add shouldHave as well!

I'm of the school that you cannot stop bad developers abusing things, so why try? That's not the job of a good library. A good library should stop you making mistakes, not try to stop you deliberately making a hash of it, as that's impossible. As soon as you try, you just end up with a restrictive and annoying library.

[helmbold]

I have to admit that 3 shouldBe lessThan(4) looks better than 3 should beLessThan(4). So, yes, maybe we need a bigger API here. But let's make sure that all imaginable combinations are working.

[sksamuel]

As long as we have a grammar of the form:

assertion:= value keyword matcher | value 'shouldBe' value
value:= literal of T | variable of T
keyword:= 'should' | 'shouldBe' | 'shouldHave' | 'shouldSomethingElse'
matcher:= Matcher<T>

Then I think its regular.

[sksamuel]

@helmbold its been a week just discussing this should vs shouldBe. I want to release KotlinTest 2 before Kotlin 1.2 comes out :)

You seem a bit happier with shouldBe. Lets release?

[helmbold]

shouldBe would be ok, but the API is still not satisfying. And it is still not possible to write "" shouldBe empty(), "" shouldBe emptyString() or "" shouldBe empty<String>(). Take a look at the unhelpful syntax completion suggestions for " " shouldBe.

Even if you'd provide a . you won't get helpful code completion:

You get much better code completion with the good, old AssertJ.

I see the matchers API as a weakness of KotlinTest. If you want to release KotlinTest 2.0 in a few days (and I want to get it out of the door, too!), I'd vote for separating the matchers API and realsing KotlinTest 2.0 without matchers.

I've experimented with a more "classic" (and less DSL-style) approach like this:

list.should.contain(2).haveSize(3).equal(listOf(1, 2, 3)).beEmpty

However my matcher library is far from being complete.

[sksamuel]

Well the matchers auto completed until you made them all top level functions :)

…

On 27 Mar 2017 20:21, "Christian Helmbold" ***@***.***> wrote: shouldBe would be ok, but the API is still not satisfying. And it is still not possible to write "" shouldBe empty(), "" shouldBe emptyString() or "" shouldBe empty<String>(). Take a look at the unhelpful syntax completion suggestions for " " shouldBe. [image: shouldbe] <https://cloud.githubusercontent.com/assets/5079980/24373385/868672bc-1331-11e7-85a3-8b6aeacaca93.png> Even if you'd provide a . you won't get helpful code completion: [image: shouldbe2] <https://cloud.githubusercontent.com/assets/5079980/24373769/bfea4ff0-1332-11e7-953a-fe8ae36d2ef2.png> You get much better code completion with the good, old AssertJ. I see the matchers API as a weakness of KotlinTest. If you want to release KotlinTest 2.0 in a few days (and I want to get it out of the door, too!), I'd vote for separating the matchers API and realsing KotlinTest 2.0 without matchers. I've experimented with a more "classic" (and less DSL-style) approach like this: list.should.contain(2).haveSize(3).equal(listOf(1, 2, 3)).beEmpty However my matcher library is far from being complete. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtZGo9FPeVoLMqP-5KZCreiK715uL_hks5rqAwrgaJpZM4Lk__m> .

[helmbold]

That is a bad side effect, I hadn't thought of. However, I think the structural improvement was right. Think, for example, if someone wants to use another matcher lib and doesn't want to be confused by the shipped matchers. But the result in the current form is not satisfying. I see several options:

• leave it as is (besides minor improvements)
• redesign the matchers API (and delay KotlinTest 2.0 further)
• remove the matchers API and deliver it separately and improve/redesign the API after KotlinTest 2.0

[sksamuel]

Kotlintest 2 is already released as we couldn't wait any longer. I mentioned it in the issues on github and I also spoke about it in the gitter room. I'm sorry if we crossed our wires on it and you didn'tsee the discussion. I thought you might have beeb away or something. But onto the main discussion. I think our matchers are the key part of the product. The specs are nice but even junit isn't that bad for laying out tests. It's about the whole experience and matchers are a part of that along with table testing and eventually and so on. People can still easily use other matchers with no issue and I don't think we should worry about other products over our own. We should be aiming for kotlintest matchers to be the default go to option in Kotlin and if not why not. Are they not good enough. If so lets improve them not think about removing them or splitting them. Imo of course.

…

On 28 Mar 2017 7:02 pm, "Christian Helmbold" ***@***.***> wrote: That is a bad side effect, I hadn't thought of. However, I think the structural improvement was right. Think, for example, if someone wants to use another matcher lib and doesn't want to be confused by the shipped matchers. But the result in the current form is not satisfying. I see several options: - leave it as is (besides minor improvements) - redesign the matchers API (and delay KotlinTest 2.0 further) - remove the matchers API and deliver it separately and improve/redesign the API after KotlinTest 2.0 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtZGuWy8r9JTW4vuGNByiWOluz-odhoks5rqUs5gaJpZM4Lk__m> .

[helmbold]

I've somehow missed the 2.0 release (and didn't expect it, since the 2.0 branch is still there and there were open issues). I think we can close this issue now.

[sksamuel]

We just needed to get the release out as we had loads of changes just building up. We can still have the discussion about matchers and then make changes in 2.1 2.2 3.0 or 100.0 if any are needed.