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

Use TestControl in tests #2651

Merged
merged 15 commits into from
Nov 30, 2021
Merged

Conversation

danicheg
Copy link
Member

@danicheg danicheg commented Sep 29, 2021

This replaces custom deterministic runtime and uses new TestControl from cats-effect in tests.
@SystemFw your thoughts on this would be so much appreciated.

@danicheg danicheg marked this pull request as ready for review November 28, 2021 11:55
Copy link
Member

@mpilquist mpilquist left a comment

Choose a reason for hiding this comment

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

Looks like the CE3 upgrade has caused one of the io tests to fail on Scala.js /cc @armanbilge

@armanbilge
Copy link
Member

I proposed a fix in #2651 (comment).

@danicheg
Copy link
Member Author

I can apply the patch 843ff4a if we agree on it. Speaking for myself - I see no problem with it.

@mpilquist
Copy link
Member

I don't think that's the same thing. I'm talking about the CI failure:

==> X fs2.io.IoPlatformSuite.toDuplexAndRead 0.09s munit.FailException: Failing seed: RUB4zd5KUZxR7R91LcjD6YarbRlLs9ray67RY2BBenL=
5861
You can reproduce this failure by adding the following override to your suite:
5862

5863
  override def scalaCheckInitialSeed = "RUB4zd5KUZxR7R91LcjD6YarbRlLs9ray67RY2BBenL="
5864

5865
Exception raised on property evaluation.
5866
> ARG_0: Stream(..)
5867
> ARG_1: Stream(..)
5868
> Exception: fs2.io.StreamDestroyedException: null

@armanbilge
Copy link
Member

Woops, commented without looking :) I will investigate that.

@mpilquist
Copy link
Member

@danicheg Sounds good, that patch looks good to me too.

@danicheg
Copy link
Member Author

Unfortunately, the error on JS preserves. It seems some changes in main after df62117 are breaking with CE 3.3.0 (CI was green at df62117). /cc @armanbilge

@armanbilge
Copy link
Member

Yeah, I worked on it a bit but confounded at the moment, I'll try again later. @danicheg if you have bandwidth to bisect CE3 to figure out what caused this here that would be ✨

@armanbilge
Copy link
Member

I bisected down to typelevel/cats-effect#2411 about cancellation, which seems plausible since the error is StreamDestroyedException and the cancellation finalizer here destroys the Node.js stream. So now I get to do /cc @vasilmkd 😉 for any pointers about your PR, please and thank you! :)

@djspiewak
Copy link
Member

@armanbilge That PR actually does quite a bit. Tldr there was a bug in IOFiber where cancelation recreated the runloop and failed to shift. This PR, together with an additional one, fixed that issue. I don't suppose it's possible to bisect further? The behavior as it stands should be correct, and was incorrect previously, but I can't imagine how you could have been depending on the broken behavior.

@armanbilge
Copy link
Member

@djspiewak that was my first instinct to bisect further, but that PR is a single commit.

@djspiewak
Copy link
Member

Since this is JS, is it at all possible that you were somehow relying on the fact that cancelation did not shift? Like, you could call cancel and it would remain on the current thread, even across multiple finalizers, if it didn't happen to hit an async boundary.

@armanbilge
Copy link
Member

I must have? Certainly not intentionally, but seems plausible. I need to refresh myself with this code.

@armanbilge
Copy link
Member

@danicheg I have another patch for you.

diff --git a/io/js/src/main/scala/fs2/io/ioplatform.scala b/io/js/src/main/scala/fs2/io/ioplatform.scala
index 57b2df3f..9dab4d6a 100644
--- a/io/js/src/main/scala/fs2/io/ioplatform.scala
+++ b/io/js/src/main/scala/fs2/io/ioplatform.scala
@@ -209,6 +209,7 @@ private[fs2] trait ioplatform {
           new streamMod.Duplex(
             streamMod
               .DuplexOptions()
+              .setAutoDestroy(false)
               .setRead { (duplex, _) =>
                 val readable = duplex.asInstanceOf[streamMod.Readable]
                 dispatcher.unsafeRunAndForget(

No idea why the CE3 change exposed this.

@danicheg
Copy link
Member Author

@armanbilge Thanks! It seems failed test is passing well locally.

@mpilquist mpilquist merged commit 6af2201 into typelevel:main Nov 30, 2021
@danicheg danicheg deleted the using-test-control branch November 30, 2021 12:55
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.

6 participants