Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some cleaner way to invert assertions #448

Open
hakanai opened this issue Mar 23, 2023 · 3 comments
Open

Some cleaner way to invert assertions #448

hakanai opened this issue Mar 23, 2023 · 3 comments

Comments

@hakanai
Copy link

hakanai commented Mar 23, 2023

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
Copy link

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

@JakeWharton
Copy link
Contributor

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
Copy link
Collaborator

evant commented May 8, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants