Add tests to `RandomSpec`, audit `Random` for platform-specific methods

[armanbilge]

See https://github.com/typelevel/cats-effect/pull/2906#issue-1179909799.

This is similar to the Console issues in https://github.com/typelevel/cats-effect/pull/2604. However, in that case the method would link and only fail at runtime.

In this case, these methods would not link at all. So arguably we could completely remove them from the JS side without breaking binary-compatibility: they did not link, and they still won't link. However, this would be source-breaking of course.

There's still the caveat if someone was making these methods usable by shimming the JDK themselves in their application so that everything would link. This would be a not-so-nice surprise for them 😅

[armanbilge]

Marked this one first-good-issue :) the key part is adding tests to RandomSpec. They don't have to be fancy at all, they just have to call each of the methods to sure that they compile/link correctly on both JVM/JS.

The tests will help us figure out which methods don't work on JS. Once we identify what those are, we can create a plan how to fix that.

[pakkeiC]

Hello @armanbilge , I would like to help on this issue. I am pretty new to the project. Could you give me some guidance on how to proceed? Thanks!

[armanbilge]

Hi @pakkeiC :) that's wonderful, thank you very much and welcome!

The first goal of this issue is to add more tests for Random to RandomSpec. So far, there is only one test there: https://github.com/typelevel/cats-effect/blob/f51d87df608cabeb781f0eb2d131236d0aa408c8/tests/shared/src/test/scala/cats/effect/std/RandomSpec.scala#L23-L30

I added that test in https://github.com/typelevel/cats-effect/pull/2906 after we got a bug report in https://github.com/typelevel/cats-effect/issues/2902 that the Random.javaSecuritySecureRandom method actually wasn't working on the JavaScript platform.

So now, my concern is that there are more methods that are broken. By adding a simple test for each method in Random, we can check if it actually works :)

After we have a test for each method, we may discover some of them are failing (I have some suspicions about that). That's okay! Once we know which ones are broken, we can make a plan how to fix them. That is the second part of this issue, but we don't have to worry about it yet.

Thanks again, and let me know if that makes sense!

[pakkeiC]

Thanks! @armanbilge It totally make sense to me. Let me get back to you when I add more method in the RandomSpec test

[armanbilge]

Sounds good! Feel free to open a (draft) PR whenever you have some questions or want feedback, and I'd be more than happy to help :)

[filipwiech]

Hi @armanbilge, I wanted to try and take a stab at it, but it seems that the RandomSpec has been expanded since this issue was originally created. Is this ticket still valid? 🙂

[armanbilge]

@filipwiech Thanks! I think there are still several untested methods and constructors, improving test coverage is always good. I think I'm more confident now that all of it is working correctly on JVM/JS/Native, so we probably won't need to make any adjustments for that.

[filipwiech]

Thanks, in that case I can take a look and try to improve the coverage, althought not sure if it should be still part of this issue or if you'd rather close it. 🙂