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

Enable Wasm tests #574

Merged
merged 1 commit into from
Jun 23, 2024
Merged

Conversation

krzema12
Copy link
Contributor

@krzema12 krzema12 commented Jun 17, 2024

Two changes were needed:

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

@krzema12 krzema12 changed the title Enable Wasm tests Enable JS and Wasm tests Jun 17, 2024
@krzema12 krzema12 changed the title Enable JS and Wasm tests Enable Wasm tests Jun 18, 2024
@krzema12 krzema12 marked this pull request as ready for review June 18, 2024 12:56
@krzema12 krzema12 force-pushed the enable-running-wasm-tests branch from 64351b7 to 8cda171 Compare June 18, 2024 12:57
@krzema12
Copy link
Contributor Author

@charleskorn ready to review!

@OptimumCode
Copy link
Contributor

OptimumCode commented Jun 21, 2024

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

Thanks @OptimumCode! Will try it.

@OptimumCode
Copy link
Contributor

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

@OptimumCode OptimumCode mentioned this pull request Jun 21, 2024
@krzema12
Copy link
Contributor Author

krzema12 commented Jun 21, 2024

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

@OptimumCode
Copy link
Contributor

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

Copy link
Owner

@charleskorn charleskorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @krzema12!

@charleskorn charleskorn merged commit 59a42fe into charleskorn:main Jun 23, 2024
1 check passed
@krzema12 krzema12 deleted the enable-running-wasm-tests branch June 23, 2024 07:05
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.

3 participants