Skip to content

Conversation

@srowen
Copy link
Member

@srowen srowen commented Sep 15, 2014

(HT @vanzin) Whatever the reason was for having this test class in main, if there is one, appear to be moot. This may have been a result of earlier streaming test reorganization.

This simply puts MasterFailureTest back under test/, removes some redundant copied code, and touches up a few tiny inspection warnings along the way.

@vanzin
Copy link
Contributor

vanzin commented Sep 15, 2014

+1 yay

@SparkQA
Copy link

SparkQA commented Sep 15, 2014

QA tests have started for PR 2399 at commit 3909411.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 15, 2014

QA tests have finished for PR 2399 at commit 3909411.

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

@pwendell
Copy link
Contributor

I'm guessing this is in the main package because it is an integration test rather than a unit test. I.e. it has a main method and is run using the full Spark assembly. If this is in the test package only then it's not possible to run it on a normal spark distribution. @tdas probably knows more, but my guess is that is why it is like this.

@vanzin
Copy link
Contributor

vanzin commented Sep 16, 2014

@pwendell FailureSuite seems to already call into all the tests exposed by MasterFailureSuite, which is why I filed the bug. The two files could probably even be merged, if that's all the needed coverage.

@srowen
Copy link
Member Author

srowen commented Sep 22, 2014

@pwendell compute-classpath will put test classes on the classpath for spark-submit et al if SPARK_TESTING=1, which is set by test invocations. This is how SparkSubmitSuite works for example, invoking its own SimpleApplicationTest class which is in src/test too. I don't think that's it, and don't see any use of MasterFailureTest except in FailureSuite.

Unless we're missing something else, I agree with @vanzin that this should probably be merged (and main removed) as perhaps this is a hold-over from an earlier usage of the test that is no longer applicable.

This PR just moves MasterFailureTest but I could go further if there's agreement.

@tdas
Copy link
Contributor

tdas commented Sep 23, 2014

Originally, if I remember correctly, this was present in non-test because I used to execute the MasterFailureTest directly using spark-class (aah, those pre Spark 1.0 days) to run longer versions of the tests that the unit test runs. But such integration-like tests should probably done as part of the spark-perf framework. And I certainly haven't run the MasterFailureTest directly in almost 1.5 years. So I am okay with this move. Change looks LGTM. But I will wait for @pwendell thoughts on this before merging.

@tdas
Copy link
Contributor

tdas commented Sep 25, 2014

okay. LGTM. i am merging this.

@asfgit asfgit closed this in c3f2a85 Sep 25, 2014
@srowen srowen deleted the SPARK-2932 branch September 26, 2014 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants