charleskorn / kaml

YAML support for kotlinx.serialization
Apache License 2.0
490 stars 47 forks source link

Enable Wasm tests #574

Closed krzema12 closed 3 weeks ago

krzema12 commented 3 weeks ago

Two changes were needed:

JS tests still fail, mostly with AssertionFailedError: expected:<NaN> but was:<NaN>, so leaving them disabled.

krzema12 commented 3 weeks ago

@charleskorn ready to review!

OptimumCode commented 3 weeks ago

Hi, @krzema12 To fix the tests with NaN you should change the assertion to something like this:

if (expectedResult.isNaN()) {
    result.isNaN() shouldBe true
} else {
    result shouldBe expectedResult
}

Because this is not correct to compare NaN to itself. This only works for jvm because the assertion uses equals method on NaN. Here is example in Java (but this also true for Kotlin):

class Scratch {
    public static void main(String[] args) {
        System.out.println(Float.NaN == Float.NaN); // false
        System.out.println(Objects.equals(Float.NaN, Float.NaN)); // true
    }
}

But in JS, NaN is always not equal to another NaN

krzema12 commented 3 weeks ago

Thanks @OptimumCode! Will try it.

OptimumCode commented 3 weeks ago

What for the rest of the failing tests:

I believe the only thing we can do here is to extract them into separate test blocks and disable them for JS target. Disabling can be done like this:

import io.kotest.common.Platform
import io.kotest.common.platform
import io.kotest.core.test.Enabled

test("throws an appropriate exception").config(enabledOrReasonIf = {
    if (platform == Platform.JS) {
        Enabled.disabled("valid floating points for JS")
    } else {
        Enabled.enabled
    }
})

The reason for this is JS... 0x2 and 0o2 are valid numbers with floating points in JS. Here is how String is converted to Double (conversion to Float uses the same method and casts value for Float):

public actual fun String.toDouble(): Double = (+(this.asDynamic())).unsafeCast<Double>().also {
    if (it.isNaN() && !this.isNaN() || it == 0.0 && this.isBlank())
        numberFormatError(this)
}

Basically, every valid number is a valid number with floating point in JS...

But one thing I cannot understand here is how those tests pass for the wasmJs target. This is a mystery for me at the moment

krzema12 commented 3 weeks ago

I suggest focusing on Wasm in this PR, to deliver some value quicker, and let's tackle JS tests separately.

OptimumCode commented 3 weeks ago

Sure, I absolutely agree with you. Just wanted to leave some notes because you said there were some issues with JS tests.