[SPARK-1010] Clean up uses of System.setProperty in unit tests

[JoshRosen]

Several of our tests call System.setProperty (or test code which implicitly sets system properties) and don't always reset/clear the modified properties, which can create ordering dependencies between tests and cause hard-to-diagnose failures.

This patch removes most uses of System.setProperty from our tests, since in most cases we can use SparkConf to set these configurations (there are a few exceptions, including the tests of SparkConf itself).

For the cases where we continue to use System.setProperty, this patch introduces a ResetSystemProperties ScalaTest mixin class which snapshots the system properties before individual tests and to automatically restores them on test completion / failure. See the block comment at the top of the ResetSystemProperties class for more details.

[JoshRosen]

/cc @nchammas @srowen if either of you know how, it might be nice to fix either https://issues.apache.org/jira/browse/SPARK-750 or https://issues.apache.org/jira/browse/SPARK-4442 so that we can use this same fixture in streaming, mllib, etc.

[JoshRosen]

Also, for some context, here's an implicit usage of System.setProperty which caused a hard-to-debug test failure: #3561 (comment)

[SparkQA]

Test build #24618 has started for PR 3739 at commit 25bfce2.

• This patch merges cleanly.

[JoshRosen]

Note to self: I should probably add a comment here explaining the ordering concerns between ResetSystemProperties and other traits that implement withFixture. For example, if you're also using BeforeAndAfterEach to modify properties, then you should order the mixins like with ResetSystemProperties with BeforeAndAfterEach so that this mixin can do the final cleanup.

[SparkQA]

Test build #24618 has finished for PR 3739 at commit 25bfce2.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24618/
Test FAILed.

[zsxwing]

Ideally, a test should not modify environment shared by other tests. Or we cannot make the tests run in parallel.

[JoshRosen]

That looks like a legitimate test failure. It looks like the issue there was that ResetSystemProperties appeared after BeforeAndAfter. The trickiness of this trait ordering might be an argument against this approach.

[JoshRosen]

Ideally, a test should not modify environment shared by other tests. Or we cannot make the tests run in parallel.

We can achieve parallelism by running multiple tests in separate JVMs. This PR doesn't introduce any new forms of sharing between tests; it only attempts to fix bugs in cases where we have such sharing but don't clean up properly (e.g. by setting some system properties then forgetting to clear them or to change them back to the old values).

[zsxwing]

The trickiness of this trait ordering might be an argument against this approach.

Yeah. But if it's well documented, I won't be against it. Really a convenient feature.

[SparkQA]

Test build #24797 has started for PR 3739 at commit 0236d66.

• This patch merges cleanly.

[JoshRosen]

Alright, I've updated this PR to remove nearly all uses of System.setProperty and replace them with SparkConf.

After this, there are only a handful of cases where we set properties in Spark core:

core/src/main/scala/org/apache/spark/SparkContext.scala:  if (master == "yarn-client") System.setProperty("SPARK_YARN_MODE", "true")
core/src/main/scala/org/apache/spark/deploy/FaultToleranceTest.scala:  System.setProperty("spark.driver.host", "172.17.42.1") // default docker host ip
core/src/main/scala/org/apache/spark/deploy/FaultToleranceTest.scala:    System.setProperty("spark.driver.port", "0")
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:      System.setProperty(key, value)
core/src/main/scala/org/apache/spark/deploy/history/HistoryServerArguments.scala:        System.setProperty("spark.history.fs.logDirectory", value)
core/src/main/scala/org/apache/spark/scheduler/cluster/YarnSchedulerBackend.scala:      System.setProperty("spark.ui.proxyBase", proxyBase)
core/src/test/scala/org/apache/spark/SparkConfSuite.scala:    System.setProperty("spark.test.testProperty", "2")
core/src/test/scala/org/apache/spark/SparkConfSuite.scala:    System.setProperty("spark.test.testProperty", "2")
core/src/test/scala/org/apache/spark/SparkConfSuite.scala:    System.setProperty("spark.test.a", "a")
core/src/test/scala/org/apache/spark/SparkConfSuite.scala:    System.setProperty("spark.test.a.b", "a.b")
core/src/test/scala/org/apache/spark/SparkConfSuite.scala:    System.setProperty("spark.test.a.b.c", "a.b.c")
core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala:    System.setProperty("spark.testing", "true")
core/src/test/scala/org/apache/spark/scheduler/SparkListenerSuite.scala:    System.setProperty("spark.akka.frameSize", "1")
core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala:    System.setProperty("os.arch", "amd64")
core/src/test/scala/org/apache/spark/util/AkkaUtilsSuite.scala:    System.setProperty("spark.hostPort", hostname + ":" + boundPort)
core/src/test/scala/org/apache/spark/util/AkkaUtilsSuite.scala:    System.setProperty("spark.hostPort", hostname + ":" + boundPort)
core/src/test/scala/org/apache/spark/util/AkkaUtilsSuite.scala:    System.setProperty("spark.hostPort", hostname + ":" + boundPort)
core/src/test/scala/org/apache/spark/util/AkkaUtilsSuite.scala:    System.setProperty("spark.hostPort", hostname + ":" + boundPort)
core/src/test/scala/org/apache/spark/util/SizeEstimatorSuite.scala:    System.setProperty("os.arch", "amd64")
core/src/test/scala/org/apache/spark/util/SizeEstimatorSuite.scala:    System.setProperty("spark.test.useCompressedOops", "true")
core/src/test/scala/org/apache/spark/util/SizeEstimatorSuite.scala:    System.setProperty("os.arch", "x86")
core/src/test/scala/org/apache/spark/util/SizeEstimatorSuite.scala:    System.setProperty("os.arch", "amd64")
core/src/test/scala/org/apache/spark/util/SizeEstimatorSuite.scala:    System.setProperty("spark.test.useCompressedOops", "false")
core/src/test/scala/org/apache/spark/util/UtilsSuite.scala:      System.setProperty("spark.test.fileNameLoadB", "2")

Going through these:

• FaultToleranceTest isn't a regular unit test.
• SparkSubmit sets some properties, but I've guarded against this with the ResetSystemProperties mixin.
• SparkConfSuite needs to set properties since it's testing SparkConf itself.
• Yarn and the HistoryServer, similarly, need to set properties.
• AkkaUtilsSuite is relatively new and actually uses SparkConf, so I assume there's a valid reason for it's use of System.setProperty.
• BlockManagerSuite and SizeEstimatorSuite may need to set the architecture property because third-party code reads it.

I'd love to get some additional eyes on these changes just to make sure that I haven't inadvertently broken anything. I intend to merge this into all of the backport branches.

[nchammas]

I'm not in a position to review this PR this week, but I just wanted to say nice work! I'm looking forward to trying out these refactored tests as part of #3564.

[SparkQA]

Test build #24797 has finished for PR 3739 at commit 0236d66.

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

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24797/
Test PASSed.

[JoshRosen]

/cc @pwendell @andrewor14 @tdas Could one of you review this? It's blocking a couple of other PRs that I'd like to merge. This looks like a lot of changes, but they're isolated to test code and most cases are small, local changes to replace system property usage with SparkConf.

[pwendell]

LGTM - I took a quick pass and everything looks good. We also talked offline about the general structure of this and I'm +1 on it.

[tdas]

LGTM for the streaming changes as well.

[JoshRosen]

I'm going to merge this into master (1.3.0) to unblock some PRs that are depending on this cleanup. I'll work on backports to the other branches, too, since some of the fixes here to SparkSubmitSuite are needed to avoid build-breaks when backporting patches that touch that code.

[JoshRosen]

Hmm, it looks like this may have caused a test in TaskResultGetterSuite to fail; let me dig in to see whether this was caused by any changes between the time that I last tested this PR and when I merged it today. If I can't figure it out in a few minutes, I'll revert this.

[JoshRosen]

Actually, this looks like it might be a non-deterministic failure. I'll keep digging.

[JoshRosen]

It doesn't look like there are any new test failures / flakiness that can be attributed to this patch, so I've finished backporting this to branch-1.2 (1.2.1), branch-1.1 (1.1.2), and branch-1.0 (1.0.3).