Skip to content

Conversation

@sarutak
Copy link
Member

@sarutak sarutak commented Mar 2, 2021

What changes were proposed in this pull request?

This PR proposes a new feature that allows developers to debug test code using JDWP with sbt an Maven.
More specifically, this PR introduces the following profile options.

  • jdwp-test-debug: An profile which controls enable/disable JDWP debug
  • test.jdwp.address: An option which corresponds to address option in JDWP
  • test.jdwp.suspend: An option which corresponds to suspend option in JDWP
  • test.jdwp.server: An option which corresponds to server option in JDWP
  • test.debug.suite: An option which controls whether debug ScalaStyle suites (Maven only)

For sbt, this feature can be used like build/sbt -Pjdwp-test-debug -Dtest.jdwp.address=localhost:9876 -Dtest.jdwp.suspend=y -Dtest.jdwp.server=y and can be used for both JUnit tests and ScalaTest tests.

For Maven, this feature can be used like as follows:

(For JUnit tests) build/mvn -Pjdwp-test-debug -Dtest.jdwp.address=localhost:9876 -Dtest.jdwp.suspend=y -Dtest.jdwp.server=y
(For ScalaTest suites) build/mvn -Pjdwp-test-debug -Dtest.debug.suite=true -Dtest.jdwp.address=localhost:9876 -Dtest.jdwp.suspend=y -Dtest.jdwp.server=y (It might be useful to specify specific sub-modules like -pl sql/core,sql/catalyst).

Why are the changes needed?

It's useful to debug test code.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

I confirmed the following things.

  • jdwp-tes-debug can switch JDWP enabled/disabled
  • test.jdwp.address can change address and port.
  • test.jdwp.suspend can change the behavior that the target debugee suspends or not.
  • test.jdwp.server can change the behavior that the JDWP debugger run as a server or client.
  • ScalaTest suites can be debugged with Maven with setting test.debug.suite to true.

@SparkQA
Copy link

SparkQA commented Mar 2, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40218/

@SparkQA
Copy link

SparkQA commented Mar 2, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40218/

@SparkQA
Copy link

SparkQA commented Mar 2, 2021

Test build #135638 has finished for PR 31706 at commit a609cd9.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thanks, @sarutak . Is there a reason not to do this in Maven?

@github-actions github-actions bot added the BUILD label Mar 2, 2021
@sarutak
Copy link
Member Author

sarutak commented Mar 2, 2021

@dongjoon-hyun It's just because I don't know how to do the same thing for Maven for now. I'll add the same feature after I find the way.

@srowen
Copy link
Member

srowen commented Mar 2, 2021

It would just be a new profile that modifies some test runner settings or something

@sarutak sarutak changed the title [SPARK-34590][TESTS] Allow JDWP debug for tests with sbt [SPARK-34590][TESTS] Allow JDWP debug for tests Mar 3, 2021
@sarutak
Copy link
Member Author

sarutak commented Mar 3, 2021

@srowen
Thanks for the advice. Now we can switch this feature with -Pjdwp-test-debug both for sbt and Maven.

@dongjoon-hyun
For Maven, scalatest-maven-plugin supports JDWP debug but it's a little bit unuseful (suspended for each sub-modules even though a sub-module doesn't contain suites to run) so I've added test.debug.suite property only for Maven.

@srowen
Copy link
Member

srowen commented Mar 3, 2021

I think it's OK to just add for SBT.

@SparkQA
Copy link

SparkQA commented Mar 3, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40265/

@SparkQA
Copy link

SparkQA commented Mar 3, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40265/

@SparkQA
Copy link

SparkQA commented Mar 3, 2021

Test build #135683 has finished for PR 31706 at commit d0461ee.

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

@sarutak
Copy link
Member Author

sarutak commented Mar 3, 2021

I think it's OK to just add for SBT.

I usually use SBT so it's OK for me but @dongjoon-hyun , do you think it's OK even if this change doesn't contain this feature for Maven?

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @sarutak .
Sure, I agree that the AS-IS approach is enough.

@dongjoon-hyun
Copy link
Member

Merged to master. Thank you, @sarutak and @srowen .

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.

4 participants