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

Add matchers for Result #836

Closed
sksamuel opened this issue Jun 17, 2019 · 9 comments · Fixed by #861
Closed

Add matchers for Result #836

sksamuel opened this issue Jun 17, 2019 · 9 comments · Fixed by #861
Labels
enhancement ✨ Suggestions for adding new features or improving existing ones. good-first-issue 👶 Suitable for newcomers looking to contribute to the project.
Milestone

Comments

@sksamuel
Copy link
Member

Jetbrains added the Result type, a bit like the Try type from arrow.
We should add matchers for this.

@sksamuel sksamuel added enhancement ✨ Suggestions for adding new features or improving existing ones. good-first-issue 👶 Suitable for newcomers looking to contribute to the project. labels Jun 17, 2019
@sksamuel sksamuel added this to the 3.4 milestone Jun 17, 2019
@faogustavo
Copy link
Contributor

faogustavo commented Jun 23, 2019

I was trying to do this, but, unfortunately, I started to face some kotlin issues (I guess).
For some unknowing reason, the generic types influence on the Result class.
Here is the snippet that I made to test this.

/**
 * You can edit, run, and share this code. 
 * play.kotlinlang.org 
 */

fun main() {
    val result = Result.runCatching { "Test 01" }
    val errorResult = Result.runCatching { throw java.io.IOException("Some error happened") }
    
    println("Test")
    println(test("Test 01", result))
    println(test("Some error happened", errorResult))
    
    println()
    println("With generics")
    println(match("Test 01", "Some error happened").test(result))
    println(match("Test 01", "Some error happened").test(errorResult))
    
    println()
    println("With kotlintest dsl-like")
    println(result should match("Test 01", "Some error happened"))
    println(errorResult should match("Test 01", "Some error happened"))
}

infix fun <T> T.should(matcher: Matcher<T>) = matcher.test(this)

fun test(expectedValue: String, result: Result<String>) = if (result.isSuccess) {
    expectedValue == result.getOrNull()
} else {
    expectedValue == result.exceptionOrNull()?.message
}

fun <T> match(expectedValue: T, errorMessage: String) = object: Matcher<Result<T>> {
    override fun test(value: Result<T>) = if (value.isSuccess) {
        print("I'm a success")
        Pair(
            value.getOrNull() == expectedValue,
            Pair(value.getOrNull(), expectedValue)
        )
    } else {
        print("I'm an error")
        Pair(
            value.exceptionOrNull()?.message == errorMessage,
            Pair(value.exceptionOrNull()?.message, errorMessage)
        )
    }
}

interface Matcher<T> {
    fun test(value: T): Pair<Boolean, Pair<Any?, Any?>>
}

The result is this 👇

Test
true
true

With generics
I'm a success - (false, (Success(Test 01), Test 01))
I'm a success - (false, (Failure(java.io.IOException: Some error happened), Test 01))

With kotlintest dsl-like
I'm a success - (false, (Success(Test 01), Test 01))
I'm a success - (false, (Failure(java.io.IOException: Some error happened), Test 01))

For some reason, when I pass the result to a generic, it creates a new "Success Result" and instead of getting "Test 01" from the .getOrNull(), its returning Success(Test 01).
The same is reproducible with the exceptions. When I send a failure, it becomes a Success(Failure(TesteException)).
I'll see if I can find some related bug on kotlin reporitory, but I think this issue will be blocked until we understand what is going on.

@faogustavo
Copy link
Contributor

faogustavo commented Jun 25, 2019

@sksamuel @Kerooker ☝️


I also create an issue on kotlin issue tracker. Didn't find any similar bug.

@LeoColman
Copy link
Member

I see.

I'll comment about this in KotlinLang's Slack, let's see if it's indeed a bug.

We can keep this on-hold for a bit :/

@Dico200
Copy link

Dico200 commented Jun 25, 2019

The type T is a Result itself. So your value in your test override is a Result<Result<*>>

@Dico200
Copy link

Dico200 commented Jun 25, 2019

Please swap the generics for concrete types and see if you can find the problem, this code is very unreadable here like this

@faogustavo
Copy link
Contributor

faogustavo commented Jun 26, 2019

Please swap the generics for concrete types and see if you can find the problem, this code is very unreadable here like this

As I showed on the code that I posted before, I already tried without generic and it works. Unfortunately, the entire library is built upon those generics and doesn't make any sense to change that. Or even create a lot of repetition just for that.

They commented on the issue on kotlin issue tracker that it's a bug on kotlin/jvm about boxing/unboxing. They already added the task as a subtask from another group and maybe will have a solution after some time.

@sksamuel
Copy link
Member Author

sksamuel commented Jul 1, 2019

What's wrong with something like this: https://pl.kotl.in/zpkSnkEeD
That works, and you can add in the shouldBeFailure complement.

@faogustavo
Copy link
Contributor

What's wrong with something like this: https://pl.kotl.in/zpkSnkEeD
That works, and you can add in the shouldBeFailure complement.

The problem is when you add one more level of complexity like this.
There is an issue that makes our syntax buggy.
If you look carefully, you will see that in the message, instead of getting Expecting Success(Test 01) to be Wibble, we go Expecting Success(Success(Test 01)) to be Wibble. There is an "extra" Success being "added" when the value is passed as a parameter with all those levels using generics.

@sksamuel
Copy link
Member Author

sksamuel commented Jul 1, 2019

We could do it without the infix version for now: https://pl.kotl.in/5RZM9cKCT
And then add the infix version once they fix the bug ?

sksamuel pushed a commit that referenced this issue Jul 3, 2019
* added result matchers

* added block as parameter and items to documentation

* used suggested workaround to tests run

* change implementation to nullable
@sksamuel sksamuel mentioned this issue Jul 3, 2019
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ Suggestions for adding new features or improving existing ones. good-first-issue 👶 Suitable for newcomers looking to contribute to the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants