ramaddepally commented on issue #23504: [SPARK-26585][K8S] Add additional integration tests for K8s Scheduler Backend URL: https://github.com/apache/spark/pull/23504#issuecomment-453307940 > I'm a little skeptical of these tests. What benefit do they bring on top of a unit test that checks that these properties are set in the pod descriptor sent to the k8s server? Setting properties and verifying they are set is very generic. I do not see any tests (unit tests or integration tests) covering memory and CPU request cases. Especially memory requests are not straight forward as we see [here](https://github.com/apache/spark/blob/master/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala#L48-L67) and [here](https://github.com/apache/spark/blob/master/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStep.scala#L52-L68). Without tests around this are, there is not guard for this code. I do not have any preference whether they should be unit test or integration tests, but there should be tests for these. > And even in the case that there is a tangible benefit, why do they need to be separate tests? Why not set these properties in an existing test? Integration tests are expensive to run, adding a bunch of small tests to check specific things means you're wasting 20-40s to test a single config property. This is something I can do (consolidate them into existing tests), if we agree there is benefit to these tests based on my above comments.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
