Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22760#discussion_r233210145
--- Diff:
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/HadoopConfExecutorFeatureStepSuite.scala
---
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import org.mockito.{Mock, MockitoAnnotations}
+import org.mockito.Matchers.{eq => Eq}
+import org.mockito.Mockito.when
+import org.mockito.invocation.InvocationOnMock
+import org.mockito.stubbing.Answer
+import org.scalatest.BeforeAndAfter
+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.deploy.k8s.{KubernetesConf,
KubernetesExecutorSpecificConf, SparkPod}
+import org.apache.spark.deploy.k8s.Constants.HADOOP_CONFIG_MAP_NAME
+import org.apache.spark.deploy.k8s.features.hadooputils.HadoopBootstrapUtil
+
+class HadoopConfExecutorFeatureStepSuite extends SparkFunSuite with
BeforeAndAfter {
+ private val hadoopConfMapName = "HADOOP_CONF_NAME"
+ private val sparkPod = SparkPod.initialPod()
+
+ @Mock
+ private var kubernetesConf:
KubernetesConf[KubernetesExecutorSpecificConf] = _
+
+ @Mock
+ private var hadoopBootstrapUtil: HadoopBootstrapUtil = _
+
+ before {
+ MockitoAnnotations.initMocks(this)
+ val sparkConf = new SparkConf(false)
+ .set(HADOOP_CONFIG_MAP_NAME, hadoopConfMapName)
+ when(kubernetesConf.sparkConf).thenReturn(sparkConf)
+ }
+
+ test("bootstrapHadoopConf being applied to a base spark pod") {
+ when(hadoopBootstrapUtil.bootstrapHadoopConfDir(
--- End diff --
> The purpose of the mocks is to make every unit test run only code that is
in the class under test
First, let me clarify that I'm against what I think is the excessive use of
mocks in these tests; and by that I mean mostly mocking a whole bunch of things
using mockito.
You can mock things without mockito. e.g. if what you want is to provide
the test with a specific mock configuration, it's pretty trivial to do that
with existing classes. Tests do this all over the code base without having to
mock `SparkConf` or any other classes.
(This is currently more painful than it should be in the k8s backend, which
is one of the things I'm trying to fix with my cleanups.)
So let's use this particular test here as an example. What is it testing?
I think it should be testing that if a user has set the appropriate
configuration in their `SparkConf`, that this step will mount a volume with the
user's Hadoop configuration in the final pod. Is that what the code here is
testing?
I don't think so. It's testing that a specific method call is being made
with certain parameters. A much better test would be to actually let the code
in the step run and do its thing, and test the final result for expected things
(in this case, the hadoop conf being mounted).
Or thinking another way: how can you actually break this test, without
removing that one method call from the step implementation? Since both the
inputs and the output of that call are mocked here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
