Kotlin bug Result unwrapping affects Atrium

[robstoll]

Affected Version

0.18.0

API

fluent-en_GB, infix-en_GB

Platform

jvm

Kotlin Version

1.4

How to reproduce the problem?

expect(Result.failure<int>(IllegalArgumentException("oh no")).toBeAFailure()

=> is success

Describe the bug

See https://youtrack.jetbrains.com/issue/KT-50974 => fix is only provided in Kotlin 1.6.20

Expected behaviour

We want that people working with Kotlin 1.4.x and 1.5.x are not affected by this Kotlin bug.
Thus, we need to implement a workaround so that the user does not get in touch with this bug (at least when using Atrium).

We can do this by comparing container.maybeSubject with the (wrong) subject provided by Kotlin. In case they differ and it is a Result.isSuccess which contains another Result, then we unwrap it.

[JordanllHarper]

I would be interested in taking this up! Where do I need to be working in and is there any resources you can send to me to help get me started?

[robstoll]

The workaround needs to be implemented in DefaultResultAssertions:

atrium/logic/extensions/atrium-logic-kotlin_1_3/src/commonMain/kotlin/ch/tutteli/atrium/logic/kotlin_1_3/impl/DefaultResultAssertions.kt

Line 37 in 190ea55

container.manualFeature(EXCEPTION) { exceptionOrNull() }.transform().let { previousExpect ->

You can read the bug report for more information but besides there is not much you can read up. Note, that this is not a good first issue, i.e. in case you are a beginner, this might be a hard one.

I suggest you base your work first on the branch https://github.com/robstoll/atrium/tree/KT-54674
And then cherry-pick to another branch which is based on main. You still need to change the Kotlin version to 1.4.32 in build.gradle and buildSrc/build.gradle.kts (you will not observe the bug on main as we are still on Kotlin 1.3.72)

[JordanllHarper]

Thanks for the info. I am still working on this, and I'm wondering how I might go about accessing the Result Kotlin provides for checking IsSuccess?
Also is doing a simple if comparison on the maybe subjects enough for comparing or is there anything else I need to access?

Apologies if these are trivial questions, just trying to figure this out!

[robstoll]

@JordanllHarper it is enough if you compare container.maybeSubject with this.exceptionOrNull in a simple if. In case you have never worked with a Option like type, you first need to map and then you can use getOrElse null.
if container.maybeSubject returns an exception but this.exceptionOrNull does not then we need to adjust.

[JordanllHarper]

@robstoll I'm still working on this and I'm wondering if I could submit a draft pull request for a first draft on the change to check I'm on the right lines. I've not gotten to unwrapping yet, just a check to see my comparisons would give the correct output.

[robstoll]

sure, you can always create a draft PR (even without asking) simplifies to understand your questions

[JordanllHarper]

Apologies if this is another simple question, but how should I be testing the fix? Are there tests to run?

[robstoll]

No worries, just keep asking, I am here to help. There is already a ResultExpectationsSpec and one test is commented out due to the bug, search for TODO.*#1234 (via regex), there are 4 places which can be re-activated. Make sure you test your fix with Kotlin 1.4

[robstoll]

fixed with #1429 #1430