Provide unit tests for SslConfigurationProvider. #183
Conversation
|
@kimoonkim @ssuchter for review |
| private val IP_ADDRESS = "192.168.99.100" | ||
| private val SSL_SECRET_DIR = s"$DRIVER_CONTAINER_SECRETS_BASE_DIR/$APP_ID-ssl" | ||
|
|
||
| private val sslFolder = Files.createTempDirectory("ssl-configuration-provider-suite").toFile |
There was a problem hiding this comment.
I couldn't quite find an elegant Scalatest equivalent to JUnit's TemporaryFolder Rule. I wonder if there's a better option than manually setting up the directory. We do this in KubernetesSuite as well.
There was a problem hiding this comment.
The current approach looks fine to me. I see many tests in core uses Utils.createTempDir, which is similar to this code:
/**
* Create a temporary directory inside the given parent directory. The directory will be
* automatically deleted when the VM shuts down.
*/
def createTempDir(
root: String = System.getProperty("java.io.tmpdir"),
namePrefix: String = "spark"): File = {
val dir = createDirectory(root, namePrefix)
ShutdownHookManager.registerShutdownDeleteDir(dir)
dir
}
Here's code snippet from SparkContextSuite:
test("basic case for addFile and listFiles") {
val dir = Utils.createTempDir()
val file1 = File.createTempFile("someprefix1", "somesuffix1", dir)
val absolutePath1 = file1.getAbsolutePath
Maybe we want to switch to this to be consistent. But, I don't have a strong preference here. i.e. I think it's your call.
kimoonkim
left a comment
kimoonkim
left a comment
There was a problem hiding this comment.
Looks great to me.
I don't have much experience with Scala unit test code, but this test code is very easy to read and understand. Thanks for writing this change. @mccheah.
I think it's good to go after addressing minor comments of mine below.
| val tempDir = Files.createTempDirectory("temp-ssl-stores").toFile() | ||
| val tempDir = Files.createTempDirectory("temp-ssl-stores").toFile | ||
| tempDir.deleteOnExit() | ||
| tempDir.deleteOnExit() |
There was a problem hiding this comment.
No need to call deleteOnExit twice at line 65 and 66?
| private val IP_ADDRESS = "192.168.99.100" | ||
| private val SSL_SECRET_DIR = s"$DRIVER_CONTAINER_SECRETS_BASE_DIR/$APP_ID-ssl" | ||
|
|
||
| private val sslFolder = Files.createTempDirectory("ssl-configuration-provider-suite").toFile |
There was a problem hiding this comment.
The current approach looks fine to me. I see many tests in core uses Utils.createTempDir, which is similar to this code:
/**
* Create a temporary directory inside the given parent directory. The directory will be
* automatically deleted when the VM shuts down.
*/
def createTempDir(
root: String = System.getProperty("java.io.tmpdir"),
namePrefix: String = "spark"): File = {
val dir = createDirectory(root, namePrefix)
ShutdownHookManager.registerShutdownDeleteDir(dir)
dir
}
Here's code snippet from SparkContextSuite:
test("basic case for addFile and listFiles") {
val dir = Utils.createTempDir()
val file1 = File.createTempFile("someprefix1", "somesuffix1", dir)
val absolutePath1 = file1.getAbsolutePath
Maybe we want to switch to this to be consistent. But, I don't have a strong preference here. i.e. I think it's your call.
|
Think I want #173 to merge first actually since that PR heavily refactors the class in question. |
|
Please rebase onto |
|
This is likely out of date with V2 submission work. |
Starting to unit test and harden everything we have so far.