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

updated to 2.13-M5 #2589

Merged
merged 13 commits into from
Nov 8, 2018
Merged

updated to 2.13-M5 #2589

merged 13 commits into from
Nov 8, 2018

Conversation

kailuowang
Copy link
Contributor

Hopefully we can merge this one quickly and publish 1.5

@kailuowang kailuowang added this to the 1.5 milestone Oct 31, 2018
@codecov-io
Copy link

codecov-io commented Oct 31, 2018

Codecov Report

Merging #2589 into master will increase coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2589      +/-   ##
==========================================
+ Coverage    94.9%   95.17%   +0.26%     
==========================================
  Files         361      361              
  Lines        6615     6653      +38     
  Branches      277      293      +16     
==========================================
+ Hits         6278     6332      +54     
+ Misses        337      321      -16
Impacted Files Coverage Δ
laws/src/main/scala/cats/laws/MonadLaws.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Ior.scala 98.54% <0%> (+0.01%) ⬆️
core/src/main/scala/cats/data/Chain.scala 99.58% <0%> (+0.01%) ⬆️
core/src/main/scala/cats/Eval.scala 98.82% <0%> (+0.04%) ⬆️
core/src/main/scala/cats/data/IorT.scala 97.79% <0%> (+0.04%) ⬆️
core/src/main/scala/cats/data/NonEmptyChain.scala 99.07% <0%> (+0.07%) ⬆️
...ore/src/main/scala/cats/data/NonEmptyMapImpl.scala 96.15% <0%> (+0.15%) ⬆️
.../src/main/scala/cats/data/RepresentableStore.scala 87.5% <0%> (+4.16%) ⬆️
core/src/main/scala/cats/syntax/either.scala 99.26% <0%> (+12.5%) ⬆️
testkit/src/main/scala/cats/tests/CatsSuite.scala 70% <0%> (+36.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a26869d...4b8db50. Read the comment docs.

@erikerlandson
Copy link
Contributor

+1, this is on the path for algebra and spire

@ChristopherDavenport
Copy link
Member

Looks like some sort of scalatest or scalactic bug.

@SethTisue
Copy link
Member

SethTisue commented Nov 1, 2018

at a glance, all that spew in the log looks like scalatest/scalatest#1423, but I think the actual failure is something else?

@kailuowang
Copy link
Contributor Author

The previous error was fixed, now I have a randomly occurring error (roughly 50% of the time) on scalajs

[info] - Try with Throwable.monadError.applicativeError recover consistent with recoverWith *** FAILED ***
[info] UndefinedBehaviorError was thrown during property evaluation.
[info] Message: An undefined behavior was detected: scala.runtime.Statics$PFMarker$@26 is not an instance of scala.util.Try
[info] Occurred when passed generated values (
[info] arg0 = Failure(java.lang.Error),
[info] arg1 =
[info] )

@erikerlandson
Copy link
Contributor

log spew might be a problem unto itself here?
The job exceeded the maximum log length, and has been terminated.

@kailuowang
Copy link
Contributor Author

Minimized scalajs error example here
I am going to mark this as blocked by scala-js/scala-js#3480 and scalatest/scalatest#1423

@kailuowang
Copy link
Contributor Author

turns out to be a scala.util.Failure regression, reported by @sjrd scala/bug#11242

@SethTisue
Copy link
Member

can it be worked around for now?

@kailuowang
Copy link
Contributor Author

@SethTisue the scala.util.Failure regression can probably be circumvented by disabling the test on 2.13 temporarily. I am not sure about the scalatest output one. I'll poke around scalatest settings.

@erikerlandson
Copy link
Contributor

just one test failing for (2.13, js_build false) in OpSuite

[info] OpSuite:
[info] - Op[Function1, Char, Int].eq.antisymmetry
[info] - Op[Function1, Char, Int].eq.reflexitivity
[info] - Op[Function1, Char, Int].eq.symmetry
[info] - Op[Function1, Char, Int].eq.transitivity
[info] - Eq[Op[Function1, Char, Int]].serializable.can serialize and deserialize *** FAILED ***
[info]   ClassCastException was thrown during property evaluation.
[info]     Message: cannot assign instance of java.lang.invoke.SerializedLambda to field org.scalacheck.Gen$$anon$5.f$5 of type scala.Function2 in instance of org.scalacheck.Gen$$anon$5
[info]     Occurred when passed generated values (
[info]   
[info]     )
[info] - Op[Function1, Char, Int].category.category left identity
[info] - Op[Function1, Char, Int].category.category right identity
[info] - Op[Function1, Char, Int].category.compose associativity
[info] - Category[Op[Function1, ?, ?]].serializable.can serialize and deserialize

@SethTisue
Copy link
Member

@kailuowang
Copy link
Contributor Author

the fix to this one is to turn fork in test back to true, it was turned to false as a workaround for the scalatest issue. So, if you allow me,
(╯°□°)╯︵ ┻━┻

@erikerlandson
Copy link
Contributor

can you just disable this single test using BuildInfo.scalaVersion != "2.13.0-M5" like you did with the other test in TrySuite?

@kailuowang
Copy link
Contributor Author

I'd like to turn fork in test back to true, since that's better memory wise. Cheeseng is looking at scalatest/scalatest#1423 so I am hopeful.

@SethTisue
Copy link
Member

I'd like to turn fork in test back to true, since that's better memory wise

I'm mostly just curious, but how come it isn't better to just crank up -Xmx some more? I don't recall seeing other projects setting fork to true for this reason (unless they were doing weird things with threads and classloaders, something like that, which I wouldn't expect in this repo)

@kailuowang
Copy link
Contributor Author

@SethTisue good question! Due to the need to generate coverage report, we need to run test from root module and congregate coverage reports (although that may be hackable). Since each test run classloader is not garbage collected, it's probably better memory wise that each module's test is run in a fresh forked jvm.

@erikerlandson
Copy link
Contributor

🎉

@kailuowang kailuowang removed the Blocked label Nov 8, 2018
Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

Nice. Thank you, @kailuowang and everyone else who helped make this happen!

@@ -17,7 +17,11 @@ class TrySuite extends CatsSuite {
checkAll("Try[Int]", CoflatMapTests[Try].coflatMap[Int, Int, Int])
checkAll("CoflatMap[Try]", SerializableTests.serializable(CoflatMap[Try]))

checkAll("Try with Throwable", MonadErrorTests[Try, Throwable].monadError[Int, Int, Int])
//temporarily disable this test due to scala.util.Failure regression https://github.com/scala/bug/issues/11242
if (BuildInfo.scalaVersion != "2.13.0-M5") {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to open an issue in this repo to remind ourselves to remove this hack.

Copy link
Contributor

Choose a reason for hiding this comment

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

done: #2601

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kailuowang kailuowang merged commit 00f2a5f into typelevel:master Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants