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

Port unit tests from fs3 topic #1478

Merged

Conversation

geoffreytchew
Copy link
Contributor

Existing series/1.0 test classes were deleted and replaced with those from the topic/fs3 branch.
A few minor modifications were made:

  1. Disabled fatal warnings for test compilation because sbt-doctest generated tests generate warnings which lead to test failures.
  2. Changed "repeatN 3" test in StreamSpec.scala to pending due to frequent (albeit intermittent) failures
  3. Re-enabled three tests that are currently marked pending in topic/fs3 because they consistently passed without error when run against the series/1.0 code. The re-enabled tests are: StreamSpec.scala: "resources acquired in outer stream are released after inner streams complete", "run finalizers of inner streams first", and "when inner stream fails, inner stream finalizer run before the primary one"
  4. Removed calls to void which is not present in trait Pull in the series/1.0 branch

gchew added 4 commits May 15, 2019 10:56
…/fs3 after they were modified by IntelliJ. Enabled 3 tests that were pending that consistently succeed on the series/1.0 branch.
…ated tests generate warnings which lead to test failures
@SystemFw
Copy link
Collaborator

SystemFw commented May 24, 2019

Oh wow, that's a lot of work, thanks a lot!
Quick question, I've seen that some things were deleted (like GroupWithinSpec or ResourceCompilationSpec), were they moved to other files? (before I go review the whole thing)

@mpilquist
Copy link
Member

mpilquist commented May 24, 2019 via email

@SystemFw
Copy link
Collaborator

quick heads up: there are several test failures , which at quick glance looks like timeouts (so possibly things hanging)

@geoffreytchew
Copy link
Contributor Author

I'll take a look at the test failures


abstract class AsyncFs2Spec extends AsyncFreeSpec with Fs2SpecLike with AsyncTimeLimitedTests {
implicit val timeout: FiniteDuration = 10.seconds
Copy link
Member

Choose a reason for hiding this comment

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

Bump this back to 60 seconds -- this is likely the cause of most/all of the timeouts on Travis

Copy link
Collaborator

Choose a reason for hiding this comment

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

btw, maybe this requires a different PR, but I think we should investigate using TestContext more instead of actual time

@@ -2125,6 +2125,7 @@ class StreamSpec extends Fs2Spec {
}

"resources acquired in outer stream are released after inner streams complete" in {
pending
Copy link
Member

Choose a reason for hiding this comment

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

Odd, these are passing on series/1.x AFAIK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm commenting them out for now, but may re-add them after I address the fact that flickersOnTravis is not behaving as expected (does not exclude tests on Travis like it should)

gchew added 2 commits May 30, 2019 13:03
…y to be reflected in the test javaOptions. This fixes the issue where flickersOnTravis in Fs2Spec was always a no-op. Marked the test 'inner stream finalizer always runs before switching' as pending because it was observed hanging both in the Travis environment and locally. Re-enabled 3 tests that should run in the series/1.0 branch
@geoffreytchew
Copy link
Contributor Author

geoffreytchew commented May 30, 2019

All tests are passing after my latest round of changes. In summary: Fixed the issue with flickersOnTravis not being applied (related to: https://stackoverflow.com/questions/27486540/jvm-options-not-passed-on-to-forked-process) and marked one additional test as pending that was failing both locally and on Travis.

@mpilquist mpilquist merged commit feb19b2 into typelevel:series/1.0 May 31, 2019
@mpilquist
Copy link
Member

Awesome work!

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.

None yet

3 participants