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

[SPARK-21939][TEST] Use TimeLimits instead of Timeouts #19150

Closed
wants to merge 2 commits into from
Closed

[SPARK-21939][TEST] Use TimeLimits instead of Timeouts #19150

wants to merge 2 commits into from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Sep 6, 2017

What changes were proposed in this pull request?

Since ScalaTest 3.0.0, org.scalatest.concurrent.Timeouts is deprecated.
This PR replaces the deprecated one with org.scalatest.concurrent.TimeLimits.

-import org.scalatest.concurrent.Timeouts._
+import org.scalatest.concurrent.TimeLimits._

How was this patch tested?

Pass the existing test suites.

@dongjoon-hyun
Copy link
Member Author

Thank you for review and approval!

@jerryshao
Copy link
Contributor

LGTM, there's a typo in PR description, "Timeouts is deprecated." not "TimeLimits".

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Sep 7, 2017

Thank you so much for review, @jerryshao ! I'll fix the typo.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Sep 7, 2017

Looks stop ensures correct shutdown in BlockGeneratorSuite is dependent on interrupting - http://www.scalatest.org/release_notes/3.0.0:

If you were relying on the default behavior of interrupting a thread on the JVM in ScalaTest 2.2.x, you'll need to define an implicit val referring to a ThreadSignaler

ScalaTest looks they changed the default for good reasons but looks we should explicitly set ThreadSignaler to keep the previous behaviour more conservatively.

implicit val defaultSignaler: Signaler = ThreadSignaler

I just double checked this passes the pending tests and also checked scalatest/scalatest@bfa983b.

@SparkQA
Copy link

SparkQA commented Sep 7, 2017

Test build #81485 has finished for PR 19150 at commit ab339b3.

  • This patch fails from timeout after a configured wait of `250m`.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class DriverSuite extends SparkFunSuite with TimeLimits
  • class AsyncRDDActionsSuite extends SparkFunSuite with BeforeAndAfterAll with TimeLimits
  • class DAGSchedulerSuite extends SparkFunSuite with LocalSparkContext with TimeLimits
  • class EventLoopSuite extends SparkFunSuite with TimeLimits
  • trait StreamTest extends QueryTest with SharedSQLContext with TimeLimits with BeforeAndAfterAll

@dongjoon-hyun
Copy link
Member Author

Oh, I see. Thank you so much. I'll add that.

@dongjoon-hyun
Copy link
Member Author

BlockGeneratorSuite and StreamTest is fixed and tested.

@SparkQA
Copy link

SparkQA commented Sep 7, 2017

Test build #81494 has finished for PR 19150 at commit 678d1b2.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

Retest this please.

@srowen
Copy link
Member

srowen commented Sep 7, 2017

Looks good. Yeah I didn't update everything when I did the scalatest update for 2.12 because the patch was so big already. Yes I also met the same Thread Signaler issue.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Sep 7, 2017

Thank you for review and approval, @srowen ! :)

@SparkQA
Copy link

SparkQA commented Sep 7, 2017

Test build #81500 has finished for PR 19150 at commit 678d1b2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 7, 2017

Test build #81501 has finished for PR 19150 at commit 678d1b2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jerryshao
Copy link
Contributor

Merging to master, thanks @dongjoon-hyun .

@asfgit asfgit closed this in c26976f Sep 8, 2017
@dongjoon-hyun
Copy link
Member Author

Thank you for review and merging, @jerryshao ! Also, thank you for review and approving, @HyukjinKwon and @srowen .

@dongjoon-hyun dongjoon-hyun deleted the SPARK-21939 branch September 8, 2017 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants