willowtreeapps / assertk

assertions for kotlin inspired by assertj
MIT License
774 stars 87 forks source link

Some cleaner way to invert assertions #448

Open hakanai opened 1 year ago

hakanai commented 1 year ago

I had a test where I asserted a file exists using

assertThat(file).exists()

Then I wrote a test where I expected the file not to exist. But there is no doesNotExist() method on Assert<File>.

So locally I created my own...

fun Assert<File>.doesNotExist() = given { actual ->
    if (!actual.exists()) return
    expected("not to exist")
}

assertThat(file).doesNotExist()

But it feels like there should be a better way to deal with this in general. For cases like isNull there is isNotNull, but doing that for every single assertion will eat a lot of time.

I can certainly do this:

assertFails {
    assertThat(file).exists()
}

It works, but it feels like I'm abusing assertFails for something it wasn't designed for.

In other libraries of a similar nature, you see patterns like...

expect(x).to equal(7)
expect(x).not_to equal(7)

So what might be nice is something like,

assertThat(file).not.exists()

Or possibly,

assertThat(file).not().exists()

Depending on how fond you are of abusing val.

This could perhaps then work for all assertions, but perhaps the failure message would end up reading weirdly for some of them.

rafsanjani commented 1 year ago

@evant Do you think these invert assertions are worth adding to the core file assertions?

JakeWharton commented 1 year ago

So I have a small prototype for inverted assertions, but it has some design problems.

The biggest is assertions which combine a test and a new return value. Today I can write:

assertThat("hello" as Any).isInstanceOf(String::class).contains("ell")

Depending on where you invert the chain becomes nonsensical. This makes sense:

assertThat("hello" as Any).isInstanceOf(String::class).not().contains("hi")

but this doesn't:

assertThat("hello" as Any).not().isInstanceOf(String::class).contains("hi")

To be clear it executes fine, because not().isInstanceOf(String::class) will fail and the contains test will never run, but that behavior is not obvious.

Maybe that's an acceptable trade-off?

I'm personally still unsure as to whether we even want this as a feature.

I actually think the thing which fits into the design of the library best would be for this to be a .none { } to contrast .all { }. It also means we could add an .any { } for #450.

evant commented 1 year ago

Yeah, I think it's tricky to get this right, both with the api and good error messages. I do agree .none {}/.any {} could be a better place to start as we already have precedence for that kind of pattern.