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

[TOREE-545] Upgrade Scalatest 3.2 and Mockito 4 #211

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

pan3793
Copy link
Member

@pan3793 pan3793 commented Aug 10, 2023

This is part of #199

Co-authored-by: Luciano Resende [email protected]
Co-authored-by: Cheng Pan [email protected]

This PR aims to upgrade scalatest to 3.2 and the corresponding mockito to 4.11 to prepare support for Scala 2.13

The change is verified by examples/magic-tutorial.ipynb

image

@@ -108,7 +110,7 @@ class IOPubClientSpec extends TestKit(ActorSystem(
.build

// Mark as target being provided
doReturn(Some(mockCommCallbacks)).when(spyCommStorage)
doReturn(Some(mockCommCallbacks), Nil: _*).when(spyCommStorage)
Copy link
Member Author

@pan3793 pan3793 Aug 10, 2023

Choose a reason for hiding this comment

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

This is a Scala bug, fixed in 2.13.0. A workaround is required to make it happy with Scala 2.12.
scala/scala#7680

@pan3793
Copy link
Member Author

pan3793 commented Aug 10, 2023

@lresende would you mind taking a look at this PR? it is based on your previous in-progress work.

@lresende
Copy link
Member

@pan3793 I believe this is mostly done in #199, and I was wondering what would be best here. Contribute things in pieces because #199 updates scala do 2.13? Or should we continue adding to that PR to fix the remaining few test issues and merge #199 pr?

@pan3793
Copy link
Member Author

pan3793 commented Aug 10, 2023

@lresende this PR is ready to go(review and merge) on my side.

I was wondering what would be best here.

In short words, I intend to make each PR focus on one goal, especially, the large goal should be broken down into small/middle ones, which are easy to implement and review.

For #199, technically, I think it is still far from the goal, because

  1. I and @requaos reached a consensus that we should not drop Scala 2.12 in Spark 3.x periods, as I explained before, Spark 3.x uses Scala 2.12 in default, it rarely uses the non-default Scala version in production.

    (from @requaos) Planning forward to support both Scala 2.12 and 2.13, with a default preference for 2.12, will be most optimal.

    Scala2.13 + Spark v3.3 #199's approach totally breaks the Scala 2.12 support.

  2. coursier upgrading involves some breaking changes, Scala2.13 + Spark v3.3 #199 silently changed the logging behavior(no test coverage). It is also worth accomplishing in an independent PR.

  3. Scala2.13 + Spark v3.3 #199 contains many irrelevant changes, e.g. it upgraded Java 8 to 11, and it's hard to thoroughly review with so many lines of change.

As @requaos said, I think he also agrees to break it down.

... this is a larger effort than initially planned ...

Please let me know what I can do to prepare this PR into the singular context of Scala 2.13 support.

I can accommodate upstream changes in my fork to assist in this effort. As you see fit to divide this labor, I will gladly reduce the scope of my PR to reflect an incremental task to work more gradually towards our shared goal.

@lresende That's my opinion, my goal is to accomplish the upgrading quickly, keep code quality, and make the code change history clear. Please let me know if you have any other thoughts :)

val scalaTest = "org.scalatest" %% "scalatest" % "3.0.8" // Apache v2
val scalaTest = "org.scalatest" %% "scalatest" % "3.2.16" // Apache v2
val scalaTestMockito = "org.scalatestplus" %% "mockito-4-11" % "3.2.16.0" // Apache v2
val mockitoInline = "org.mockito" % "mockito-inline" % "4.11.0" // MIT
Copy link
Member Author

Choose a reason for hiding this comment

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

all are test only, so no need to update LICENSE/NOTICE

Copy link
Member Author

@pan3793 pan3793 Aug 10, 2023

Choose a reason for hiding this comment

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

BTW, mockito-inline is introduced to support mock final class / enum

@pan3793
Copy link
Member Author

pan3793 commented Aug 10, 2023

The test failure is irrelevant, it's another known flaky test, will investigate independently.

[info]   - should send a status idle message after the reply and result *** FAILED *** (15 seconds, 40 milliseconds)
[info]     0 did not equal 2 (ExecuteRequestHandlerSpec.scala:193)
[info]     org.scalatest.exceptions.TestFailedException:
[info]     at org.scalatest.matchers.MatchersHelper$.indicateFailure(MatchersHelper.scala:397)
[info]     at org.scalatest.matchers.should.Matchers$ShouldMethodHelperClass.shouldMatcher(Matchers.scala:7299)
[info]     at org.scalatest.matchers.should.Matchers$AnyShouldWrapper.should(Matchers.scala:7362)
[info]     at org.apache.toree.kernel.protocol.v5.handler.ExecuteRequestHandlerSpec.$anonfun$new$6(ExecuteRequestHandlerSpec.scala:193)

@pan3793 pan3793 force-pushed the TOREE-545 branch 2 times, most recently from e340c29 to ff8aa97 Compare August 11, 2023 04:04
@pan3793
Copy link
Member Author

pan3793 commented Aug 20, 2023

@lresende I opened #218 based on this PR (I have to open a new PR because I don't have permission to commit on the original PR branch), which is a rough and dirty fix of #199, it passed building and testing on both Scala 2.12 and 2.13. I would like to continue the work after this PR get in.

@pan3793
Copy link
Member Author

pan3793 commented Aug 22, 2023

@lresende do you have further concerns about shipping this patch?

@pan3793
Copy link
Member Author

pan3793 commented Aug 29, 2023

This PR only touches the test code, since all tests passed and no further objections, I'm going to merge it to unblock Scala 2.13 upgrading tasks.

@pan3793 pan3793 merged commit 7bc66ee into apache:master Aug 29, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants