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

Initial Prototype #1

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Initial Prototype #1

wants to merge 13 commits into from

Conversation

kyay10
Copy link
Collaborator

@kyay10 kyay10 commented Jan 12, 2024

This PR is here to track feedback on the initial prototype so far. API, possible suggestions, etc are very welcome! The aim is to make this library a temporary companion to arrow-core until context receivers are fully released on every platform (so that there are no pre-release binaries). Pointers to APIs in Arrow or Kotlin Stdlib that would benefit from contexts are welcome as well.

@kyay10 kyay10 self-assigned this Jan 12, 2024
@kyay10 kyay10 marked this pull request as draft January 12, 2024 03:40
@cloudshiftchris
Copy link

cloudshiftchris commented Jan 14, 2024

Using kotest to test context receiver Arrow functions doesn't feel like Arrow is a first-class citizen.

When we had Either<Error,A> as a return value we could simply call methods and use shouldBeRight() or shouldBeLeft().

With context receivers we need to setup a context to get the test compiling/executing, and need to capture the error path to validate it; happy-path result verification is cleaner as it's simply the return value.

This is one mechanism to make this work (see below for ideas on improvement):

   test("UUID converter works") {
            either {
                val uuid = UUID.randomUUID()
                converter.convert(ResolvedNodes.string(uuid.toString()), type, conversionContext())
                    .shouldBe(uuid)
            }.shouldBeRight()

            either {
                converter.convert(ResolvedNodes.string(""), type, conversionContext())
            }.shouldBeLeft()
                .shouldBeInstanceOf<ConversionError.TypeConversionError>()
                .message()
                .shouldBe(
                    "Error converting '' into 'java.util.UUID': IllegalArgumentException: Invalid UUID string: "
                )
        }

...with a few helper shims its a bit cleaner:

 test("UUID converter works") {
            shouldBeRight {
                val uuid = UUID.randomUUID()
                converter.convert(ResolvedNodes.string(uuid.toString()), type, conversionContext())
                    .shouldBe(uuid)
            }

            shouldBeLeft {
                converter.convert(ResolvedNodes.string(""), type, conversionContext())
            }.shouldBeInstanceOf<ConversionError.TypeConversionError>()
                .message()
                .shouldBe(
                    "Error converting '' into 'java.util.UUID': IllegalArgumentException: Invalid UUID string: "
                )
        }

        public inline fun <Error, A> shouldBeRight(@BuilderInference block: Raise<Error>.() -> A): A = either(block).shouldBeRight()
        public inline fun <Error, A> shouldBeLeft(@BuilderInference block: Raise<Error>.() -> A): Error = either(block).shouldBeLeft()

It would be ideal if Kotest tests using Arrow/Either/context-receivers were symmetrical with raw Kotlin code w/ exceptions, specifically:

a) There would be a mechanism to execute code in a context w/o having to explicitly (and repeatedly) establish that context;
b) As with Kotest exception handling in tests, any errors that are raised fail the test;
c) Some form of shouldRaise<ErrorType>, similar to shouldThrow<ExceptionType> to handle the error path.

Perhaps parts of this exist already and my evolving Arrow journey hasn't taken me there yet.

@kyay10
Copy link
Collaborator Author

kyay10 commented Jan 15, 2024

@cloudshiftchris Very insightful. I've added shouldRaise and shouldSucceed to the test code and they seem to clean things up well.
Those functions should be eventually added to kotest-extensions-arrow, but I'd like to try them out here first before I suggest them

@cloudshiftchris
Copy link

@kyay10 thanks for the quick turnaround!

Dropped those implementations in to compare against what we've stubbed out internally - comparison w/ commentary on a few actual tests below.

Don't think we're there yet - the mixing of "providing a context" with "checking assertions" is somewhat problematic.

This is syntactically ideal - separating context receiver from assertion - with unknowns (for me) on how to have kotest provide a context receiver:

        // doesn't compile
        // this would <somehow> provide a context for the test *and* ensure that no errors were raised 
        // (that otherwise weren't handled in the block, for example, by `shouldRaise`)
        // this would eliminate the need for the `shouldSucceed` 
        context(Raise<ConfigurationError>)
        test("sample magic kotest / arrow voodoo") {
           // uses context receiver provided above
            converter.convert(ResolvedNodes.boolean(true), type, conversionContext()).shouldBe(true)

            // symmetrical to `shouldThrow<ExceptionType>`
            // uses the context provided above - only responsibility is verifying error type
            shouldRaise<ConversionError.TypeConversionError> {
                converter.convert(ResolvedNodes.string("abc"), type, conversionContext())
            }
                .message()
                .shouldBe(
                    "Error converting 'abc' into 'kotlin.Boolean': Expected value to be in [true, t, 1, yes] or [false, f, 0, no]"
                )

            // symmetrical to `shouldThrowAny`
            // uses the context provided above - only responsibility is verifying error raised
            shouldRaiseAny {
                converter.convert(ResolvedNodes.string("abc"), type, conversionContext())
            }
        }

Comparison:

          // using our internal `shouldSucceed` implementation
            // here it's used as a means to provide a context for the enclosed block, and verify that no errors were raised.
            shouldSucceed {
                converter.convert(ResolvedNodes.boolean(true), type, conversionContext()).shouldBe(true)
                converter.convert(ResolvedNodes.boolean(false), type, conversionContext()).shouldBe(false)
                converter.convert(ResolvedNodes.string("true"), type, conversionContext()).shouldBe(true)
                converter.convert(ResolvedNodes.string("false"), type, conversionContext()).shouldBe(false)
            }

            // could also use it to further chain assertions
            shouldSucceed {
                converter.convert(ResolvedNodes.boolean(true), type, conversionContext())
            }.shouldBe(true)

            // using arrow-context `shouldSucceed` implementation
            // here it's used as a means to provide a context for the enclosed block, and verify that no errors were raised.
            // this is functionally equivalent to the above, when used solely to provide a context
            arrow.context.shouldSucceed {
                converter.convert(ResolvedNodes.boolean(true), type, conversionContext()).shouldBe(true)
                converter.convert(ResolvedNodes.boolean(false), type, conversionContext()).shouldBe(false)
                converter.convert(ResolvedNodes.string("true"), type, conversionContext()).shouldBe(true)
                converter.convert(ResolvedNodes.string("false"), type, conversionContext()).shouldBe(false)
            }

            // could also use it to further chain assertions
            arrow.context.shouldSucceed {
                converter.convert(ResolvedNodes.boolean(true), type, conversionContext())
            }.shouldBe(true)

            // not loving this overload; it seems problematic to combine `shouldBe` comparison with the
            // context provided by `shouldSucceed` - why `shouldBe` and not other assertions?
            // does this imply that only a single assertion should be made within the context?
            // perhaps we leave validation of the result to the caller, and only provide the context?
            arrow.context.shouldSucceed(true) {
                converter.convert(ResolvedNodes.boolean(true), type, conversionContext())!!
            }

            // using our internal `shouldRaise` implementation
            // this requires two type arguments (perhaps someone with better generics-fu can improve on this)
            // one argument for the type of error expected, and one (supertype) for the context to execute within
            // aside from the second type argument this is equivalent to `shouldThrow<ExceptionType>` for non-fp code
            // as with `shouldThrow` we can further chain assertions on the raised error (which is smart-casted to the expected type)
            shouldRaise<ConversionError.TypeConversionError, ConfigurationError> {
                    converter.convert(ResolvedNodes.string("abc"), type, conversionContext())
                }
                .message()
                .shouldBe(
                    "Error converting 'abc' into 'kotlin.Boolean': Expected value to be in [true, t, 1, yes] or [false, f, 0, no]"
                )

            // using arrow-context `shouldRaise` implementation
            // this doesn't compile as the expected error type is a subtype of the context type
            // nor does this `shouldRaise` implementation check that the raised error is of the expected type
            arrow.context
                .shouldRaise<ConversionError.TypeConversionError> {
                    converter.convert(ResolvedNodes.string("abc"), type, conversionContext())
                }
                .message()
                .shouldBe(
                    "Error converting 'abc' into 'kotlin.Boolean': Expected value to be in [true, t, 1, yes] or [false, f, 0, no]"
                )

            // using arrow-context `shouldRaise` implementation
            // this compiles but is not equivalent to `shouldThrow` - its the same behaviour as `shouldThrowAny`
            // this isn't equivalent to our internal `shouldRaise` implementation, as it doesn't check that the raised error is of the expected type
            // (this could be done with an additional .shouldBeInstanceOf check, but the goal was symmetric with `shouldThrow<ExceptionType>`
            arrow.context
                .shouldRaise {
                    converter.convert(ResolvedNodes.string("abc"), type, conversionContext())
                }
                .message()
                .shouldBe(
                    "Error converting 'abc' into 'kotlin.Boolean': Expected value to be in [true, t, 1, yes] or [false, f, 0, no]"
                )

            // not loving this overload, for similar reasons as noted above for `arrow.context.shouldSucceed`
            // providing an instance for `shouldBe` isn't always desired - in this case, it's problematic to create an instance of the expected error type
            // would prefer to leave further validation of the error to the caller
            // (this doesn't compile)
            arrow.context
                .shouldRaise(ConversionError.TypeConversionError(...)) {
                    converter.convert(ResolvedNodes.string("abc"), type, conversionContext())
                }
                .message()
                .shouldBe(
                    "Error converting 'abc' into 'kotlin.Boolean': Expected value to be in [true, t, 1, yes] or [false, f, 0, no]"
                )


            public inline fun <Error, A> shouldSucceed(@BuilderInference block: Raise<Error>.() -> A): A =
                recover(block) { throw AssertionError("Expected to succeed, but raised $it") }

            public inline fun <reified ExpectedError : Error, Error : Any> shouldRaise(
                @BuilderInference block: Raise<Error>.() -> Unit
            ): ExpectedError {
                recover(block) { error ->
                    if (error is ExpectedError) return error
                    throw AssertionError(
                        "Expected raise of ${ExpectedError::class.bestName()} but ${error::class.bestName()} was raised: $error"
                    )
                }
                throw AssertionError("Expected raise of ${ExpectedError::class.bestName()} but no error was raised.")
            }

@cloudshiftchris
Copy link

cloudshiftchris commented Jan 15, 2024

This is closer - adding withContext around the whole test removes the need for shouldSucceed; haven't yet figured out how to reduce shouldRaise to a single type parameter.

Shifts the "provide me a context to run in" to withContext - which really should be done by kotest, when they have a strategy for supporting context receivers; at that point would expect withContext to melt away.

 test("Boolean converter works") {
            withContext {
                converter.convert(ResolvedNodes.boolean(true), type, conversionContext())
                    .shouldBe(true)
                converter.convert(ResolvedNodes.boolean(false), type, conversionContext())
                    .shouldBe(false)
                converter.convert(ResolvedNodes.string("true"), type, conversionContext())
                    .shouldBe(true)
                converter.convert(ResolvedNodes.string("false"), type, conversionContext())
                    .shouldBe(false)
                
                shouldRaise<ConversionError.TypeConversionError, ConfigurationError> {
                    converter.convert(ResolvedNodes.string("abc"), type, conversionContext())
                }
                    .message()
                    .shouldBe(
                        "Error converting 'abc' into 'kotlin.Boolean': Expected value to be in [true, t, 1, yes] or [false, f, 0, no]"
                    )
            }
        }


        public inline fun <Error> TestScope.withContext(@BuilderInference block: Raise<Error>.() -> Unit): Unit =
            recover(block) { throw AssertionError("Expected to succeed, but raised $it") }

        public inline fun <reified ExpectedError, Error : Any> Raise<Error>.shouldRaise(
            @BuilderInference block: Raise<Error>.() -> Unit
        ): ExpectedError {
            recover(block) { error ->
                if (error is ExpectedError) return error
                throw AssertionError(
                    "Expected raise of ${ExpectedError::class.bestName()} but ${error::class.bestName()} was raised: $error"
                )
            }
            throw AssertionError("Expected raise of ${ExpectedError::class.bestName()} but no error was raised.")
        }

@kyay10
Copy link
Collaborator Author

kyay10 commented Jan 15, 2024

@cloudshiftchris Check the new commit! Thank you for the great suggestions! Hopefully, kotest can support something similar to ExtendWith from junit. I don't think there's a way to reduce shouldRaise to one type parameter. The reasons are complex, but Raise should never support a recover method that reuses an out-there Raise instance. In other words, every Raise method must create its own Raise instance if it wishes to grab an Error value out of it. Thus, shouldRaise must create its own instance, and thus it needs knowledge of the outer error type.

@cloudshiftchris
Copy link

@kyay10 amazing. thank you for the context (pun intended) around Raise - had dug into it a bit and figured it was special.

On that same topic (of requiring a specific Raise instance) - not sure if this is solvable but ran into a few challenges (of my own making) that took a bit to figure out, perhaps Arrow/compiler could help somehow.

This is a working test:

 test("Boolean converter works") {
            withContext {
                converter
                    .convert(ResolvedNodes.boolean(true), type, conversionContext())
                    .shouldBe(true)

                shouldRaise<ConversionError.TypeConversionError, ConfigurationError> {
                    converter.convert(ResolvedNodes.string("abc"), type, conversionContext())
                }
                    .message()
                    .shouldBe(
                        "Error converting 'abc' into 'kotlin.Boolean': Expected value to be in [true, t, 1, yes] or [false, f, 0, no]"
                    )
            }
        }

...however, I struggled to get it working, having accidentally done this (note the second type parameter on shouldRaise):

        test("Boolean converter works") {
            withContext {
                converter
                    .convert(ResolvedNodes.boolean(true), type, conversionContext())
                    .shouldBe(true)

                shouldRaise<ConversionError.TypeConversionError, ConversionError> {
                    converter.convert(ResolvedNodes.string("abc"), type, conversionContext())
                }
                    .message()
                    .shouldBe(
                        "Error converting 'abc' into 'kotlin.Boolean': Expected value to be in [true, t, 1, yes] or [false, f, 0, no]"
                    )
            }
        }

...because the types are all in the same hierarchy the compiler never blinked; at runtime, where shouldRaise would expect to fail, the internals never tripped as the block being executed was done so in the outer context (as the inner context was not of the correct type). Took a bit to figure out. Understand the need for individual Raise blocks - perhaps there is a way to enforce that the blocks must be of the same type.

New code looks great! A small amount of code that goes a long way.

Only a minor naming consistency item, if we desire symmetry with kotest's shouldThrowAny, then this should be shouldRaiseAny: inline fun <OuterError> shouldRaise(block: context(Raise<OuterError>) () -> Any?): OuterError

I've asked on the #kotest slack channel around context-receiver options: https://kotlinlang.slack.com/archives/CT0G9SD7Z/p1705370189026969.

@kyay10
Copy link
Collaborator Author

kyay10 commented Jan 18, 2024

@cloudshiftchris with the risk of slightly derailing the conversation, I want to point out that you can define a Raise subclass, mark it with a @DslMarker annotation, and use it in withContext and shouldRaise. This would prevent outer calls to the withContext instance from within shouldRaise. Also note that then you can define shouldRaise inside of that Raise subclass, and make its type parameter invariant, and hence you won't face this issue, and you'll have a shouldRaise that only takes 1 type argument.

Edit: See latest commit!

@cloudshiftchris
Copy link

@kyay10 thanks for the guidance & commit showing how that would work.

Back-ported for Arrow 1.2 / Kotlin 1.9.x - works well, will a minor annoyance of failure messages being nested due to two levels or Raise (DefaultRaise from recover and custom TestingRaise) in withContext2, likely due to my uncertainty on how to use the custom Raise w/o something like a RaiseResolver.

java.lang.AssertionError: Expected to succeed, but raised TypeConversionError(Error converting 'abc' into 'kotlin.Boolean': AssertionError: Expected to succeed, but raised TypeConversionError(Error converting 'abc' into 'kotlin.Boolean': Expected value to be in [true, t, 1, yes] or [false, f, 0, no]))
public inline fun <Error> TestScope.withContext2(
    @BuilderInference block: (@TestingRaiseDsl TestingRaise<Error>).() -> Unit
): Unit {
    recover<Error, Unit>({ block(TestingRaise()) }) { fail("Expected to succeed, but raised $it") }
}

@Target(AnnotationTarget.CLASS, AnnotationTarget.TYPE)
@DslMarker
public annotation class TestingRaiseDsl

public class TestingRaise<Error> : Raise<Error> {
    override fun raise(r: Error): Nothing = fail("Expected to succeed, but raised $r")

    public inline fun shouldRaiseAny(block: (@TestingRaiseDsl Raise<Error>).() -> Any?): Error {
        val result = recover(block) { error -> return error }
        fail("Expected to raise an error, but instead succeeded with $result")
    }

    public inline fun <reified ExpectedError : Any> shouldRaise(
        block: (@TestingRaiseDsl Raise<Error>).() -> Any?
    ): ExpectedError =
        shouldRaiseAny(block).shouldBeInstanceOf<ExpectedError>()

    public inline fun shouldRaise(
        expected: Error,
        block: context((@TestingRaiseDsl Raise<Error>)) () -> Any?
    ): Error =
        shouldRaiseAny(block) shouldBe expected
}

@kyay10
Copy link
Collaborator Author

kyay10 commented Jan 18, 2024

withContext2's body can simply be: block(TestingRaise())

@cloudshiftchris
Copy link

cloudshiftchris commented Jan 18, 2024

🤦 yep, overthought the heck out of that one ;) thank you.

That wasn't actually the issue, however: the messages are still nested - this likely due to my code raising an error internally, which is immediately turned into an AssertionError, however, that is in a catch( { ... code that raises ... } ) { raise an error here }

Not seeing a clean way around that, short of catch ignoring AssertionErrors.

@cloudshiftchris
Copy link

This addresses it - leaving raised errors intact until the end of the context scope (i.e. letting them bubble up as they normally would), then (if present) fail the test.


public inline fun <Error> TestScope.withContext(
    @BuilderInference block: (@TestingRaiseDsl TestingRaise<Error>).() -> Unit
): Unit {
    recover({ block(TestingRaise(this)) }) { error ->
        fail("Expected to succeed, but raised $error")
    }
}

@Target(AnnotationTarget.CLASS, AnnotationTarget.TYPE)
@DslMarker
public annotation class TestingRaiseDsl

public class TestingRaise<Error>(private val outerRaise: Raise<Error>) : Raise<Error> {
    override fun raise(r: Error): Nothing = outerRaise.raise(r)

    public inline fun shouldRaiseAny(block: (@TestingRaiseDsl Raise<Error>).() -> Any?): Error {
        val result =
            recover(block) { error ->
                return error
            }
        fail("Expected to raise an error, but instead succeeded with $result")
    }

    public inline fun <reified ExpectedError : Any> shouldRaise(
        block: (@TestingRaiseDsl Raise<Error>).() -> Any?
    ): ExpectedError = shouldRaiseAny(block).shouldBeInstanceOf<ExpectedError>()

    public inline fun shouldRaise(
        expected: Error,
        block: (@TestingRaiseDsl Raise<Error>).() -> Any?
    ): Error = shouldRaiseAny(block) shouldBe expected
}

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

Successfully merging this pull request may close these issues.

2 participants