[SPARK-21524] [ML] unit test fix: ValidatorParamsSuiteHelpers generates wrong temp files

[hhbyyh]

What changes were proposed in this pull request?

jira: https://issues.apache.org/jira/browse/SPARK-21524

ValidatorParamsSuiteHelpers.testFileMove() is generating temp dir in the wrong place and does not delete them.

ValidatorParamsSuiteHelpers.testFileMove() is invoked by TrainValidationSplitSuite and crossValidatorSuite. Currently it uses tempDir from TempDirectory, which unfortunately is never initialized since the boforeAll() of ValidatorParamsSuiteHelpers is never invoked.

In my system, it leaves some temp directories in the assembly folder each time I run the TrainValidationSplitSuite and crossValidatorSuite.

How was this patch tested?

unit test fix

[SparkQA]

Test build #79920 has finished for PR 18728 at commit dca7272.

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

[srowen]

Hm, is the more basic error here that object ValidatorParamsSuiteHelpers extends SparkFunSuite ? it seems like it's trying to leverage the temp dir cleanup function in the superclass, but it will never be invoked because this object is never used as an actual test. What about removing both of those superclasses?

[hhbyyh]

Thanks for your attention. @srowen
The temp dir cleanup function is implemented in trait DefaultReadWriteTest which extends TempDirectory, not from SparkFunSuite. And as you said, the beforeAll and afterAll in ValidatorParamsSuiteHelpers was never really triggered. Thus it's creating temp files in the runtime directory and not deleting them.

Removing the SparkFunSuite will cause the === failed in the compareParamMaps function in ValidatorParamsSuiteHelpers. Shall we remove it?

[srowen]

What about only extending, say, FunSuite? there's probably something higher in the hierarchy that would suffice, if all that's really used is ===. Tightening this up a bit might be useful.

[hhbyyh]

Yes, that's good. But I just found there's one rule in the scala style check
"Tests must extend org.apache.spark.SparkFunSuite instead."
try to ignore it?

[srowen]

How about extending just org.scalatest.Assertions? if that doesn't work, I'd just forget it, sure.

[hhbyyh]

That appears to be all right. Sending update.

[SparkQA]

Test build #79941 has finished for PR 18728 at commit 4da71ce.

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

[srowen]

merged to master