Tests need some love

[SystemFw]

First, a little puzzle:

class Sane extends AsyncFs2Spec {
  "nope!" in { IO(1).unsafeToFuture.map(_ => false shouldBe true) }
}

class InSane extends Fs2Spec {
  "I guess so..." in { IO(1).unsafeToFuture.map(_ => false shouldBe true) }
}

The first test correctly fails, but the second succeeds! This means that, at a minimum, the current RefSpec is completely broken (I'm fixing it as part of the Ref PR). I haven't investigated but there might well be other tests that should be failing and aren't. ( 😱 )

Then, I'd say we need to bring some order to the way assertions are run: some are run with unsafeRunSync, others with unsafeToFuture.map (up to the bug above), plus there are some helpers for Streams like runLog and runLogF. Ideally a single way of running them (or some guidelines on how to pick) would be great, or even better we can create a custom matcher that does what we want (I've done it for Task in specs2, don't know about scalatest).

Third, I'm a bit unsure about the criterion to put tests in jvm rather than shared, thoughts?

Fourth, if/when these points are addressed, a cleanup of the test suite in this direction needs to be done

[mpilquist]

Good catch on RefSpec. I agree tests need to be organized / cleaned up / etc.

Ideally, tests should be in shared -- however, tests which rely on unsafeRunSync don't work on Scala.js if the underlying IO has an async boundary. In such cases, we have to use unsafeToFuture with assertions in the map call on the Future. However scalacheck doesn't support async props (typelevel/scalacheck#214) so we need to call unsafeRunSync in any property based tests, which means either defining them in jvm or defining them in shared only when they don't have async steps.

[SystemFw]

Ah, the explanation makes sense actually :)
It's good to have it written here because it's certainly non-obvious

[SystemFw]

Is there any particular reason why EventuallySupport can't be in shared?

[mpilquist]

Not that I recall

…

On Dec 2, 2017, at 11:59 AM, Fabio Labella ***@***.***> wrote: Is there any particular reason why EventuallySupport can't be in shared? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[SystemFw]

Just found out the reason I think. eventually doesn't really work with Future (I think), so it can only be used in unsafeRunSync style tests, which have to be in jvm

[SystemFw]

Ok, I'm posting rough guidelines about what can be done here:

• In tests that use more than one unsafeRunSync, change them to use only one at the very end, like you would with normal code (use map and flatMap instead). The rationale is that people that look at tests to see examples should see correct fs2 code.
• In tests that use unsafeRunSync as the latest instruction, change to unsafeToFuture.map { sideEffectingAssertion}. Do note the earlier comment about scalacheck lack of support for async assertions though. Also, make sure that your tests now extend `AsyncFs2Spec (see the top of the issue to know why)
• Tests that have no unsafeRunSync at this point should be in shared.
• Review the division of tests, e.g. one file per operation, or any other criteria. This is pretty vague so there's room for suggestions.
• What's the deal with our assertions? sometimes we use unsafeToFuture directly, sometimes we use the helpers. Some consistency would help one way or another. Improving or removing the helpers should also be on the table.
• Some tests are pending. Ideally those should be addressed.
• How to test time. I think we need a test Timer either here or in cats-effect. Tests relying on time should use TextContext from cats-effect.
• Add more tests. There are several combinators that aren't really tested.

Obviously multiple people can work on this, every little helps :)
We will also help out filling any knowledge gaps if required.

• Note:
  A piece of probably unneeded advice: when modifying a test, make sure you can make it fail first (by asserting the opposite of what you mean and running it). This has caught tens of faulty tests for me.

[Daenyth]

A piece of probably unneeded advice: when modifying a test, make sure you can make it fail first (by asserting the opposite of what you mean and running it). This has caught tens of faulty tests for me.

It may be worth exploring some mutation testing.

[SystemFw]

Heh a former colleague of mine is the author of the leading mutation testing framework (PITest), it used to have a scala module ages ago, but it might not work anymore since it's byte code based. A mutation testing framework based on scalameta sounds like a nice project for someone :)

EDIT: by "tens of faulty tests" I meant in my career, not in fs2's codebase 😂

[mpilquist]

Closing this one out -- let's create some smaller / more focused issues for anything left outstanding.