Copilot commented on code in PR #6094: URL: https://github.com/apache/texera/pull/6094#discussion_r3522723489
########## common/config/src/test/scala/org/apache/texera/common/config/PythonUtilsSpec.scala: ########## @@ -0,0 +1,49 @@ +/* + * 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.texera.common.config + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Spec for [[PythonUtils]]. `getPythonExecutable` falls back to "python3" when the configured + * python path is blank, else returns the trimmed path. The blank-path default depends on + * UDF_PYTHON_PATH, so the exact-value assertion is guarded on that override being unset. + */ +class PythonUtilsSpec extends AnyFlatSpec with Matchers { + + "PythonUtils.getPythonExecutable" should "fall back to python3 when no python path is configured" in { + if (sys.env.get("UDF_PYTHON_PATH").forall(_.trim.isEmpty)) { + PythonUtils.getPythonExecutable shouldBe "python3" + } + } Review Comment: This test only checks the environment variable, but the config substitution `${?UDF_PYTHON_PATH}` can also be satisfied by a JVM system property. If -DUDF_PYTHON_PATH is set, this assertion can incorrectly expect "python3" and fail. ########## common/config/src/test/scala/org/apache/texera/common/config/PekkoConfigSpec.scala: ########## @@ -0,0 +1,75 @@ +/* + * 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.texera.common.config + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Spec for [[PekkoConfig]]. Reading keys off the returned config forces resolution of cluster.conf + * (merged with the Typesafe default application config), so a renamed or mistyped key surfaces here + * as a ConfigException. The log level carries a `${?ENV}` override, so its assertion is guarded. + */ +class PekkoConfigSpec extends AnyFlatSpec with Matchers { + + "PekkoConfig.pekkoConfig" should "load the actor/serialization settings from cluster.conf" in { + val config = PekkoConfig.pekkoConfig + config.getString("pekko.actor.provider") shouldBe "cluster" + config.getBoolean("pekko.actor.allow-java-serialization") shouldBe false + config.getBoolean("pekko.actor.enable-additional-serialization-bindings") shouldBe true + config.getString( + "pekko.actor.serializers.kryo" + ) shouldBe "io.altoo.serialization.kryo.pekko.PekkoKryoSerializer" + config.getStringList("pekko.loggers").get(0) shouldBe "org.apache.pekko.event.slf4j.Slf4jLogger" + config.getString( + "pekko.logging-filter" + ) shouldBe "org.apache.pekko.event.slf4j.Slf4jLoggingFilter" + } + + it should "expose the remote/artery transport settings" in { + val config = PekkoConfig.pekkoConfig + config.getString("pekko.remote.artery.transport") shouldBe "tcp" + config.getString("pekko.remote.artery.canonical.hostname") shouldBe "0.0.0.0" + config.getInt("pekko.remote.artery.canonical.port") shouldBe 0 + config.getBytes("pekko.remote.artery.advanced.maximum-frame-size") shouldBe 31457280L + config.getBytes("pekko.remote.artery.advanced.maximum-large-frame-size") shouldBe 125829120L + } + + it should "expose the cluster and failure-detector settings" in { + val config = PekkoConfig.pekkoConfig + config.getStringList("pekko.cluster.seed-nodes").size shouldBe 0 + config.getString( + "pekko.cluster.downing-provider-class" + ) shouldBe "org.apache.pekko.cluster.sbr.SplitBrainResolverProvider" + config.getString("pekko.cluster.gossip-interval") shouldBe "10s" + config.getString("pekko.cluster.failure-detector.acceptable-heartbeat-pause") shouldBe "50s" + config.getString( + "pekko-kryo-serialization.kryo-initializer" + ) shouldBe "org.apache.texera.amber.engine.common.AmberKryoInitializer" + } + + it should "resolve the log levels" in { + val config = PekkoConfig.pekkoConfig + if (sys.env.get("TEXERA_SERVICE_LOG_LEVEL").isEmpty) { + config.getString("pekko.loglevel") shouldBe "INFO" + } + config.getString("pekko.stdout-loglevel") shouldBe "INFO" + } Review Comment: The loglevel in cluster.conf is overrideable via `${?TEXERA_SERVICE_LOG_LEVEL}`; that substitution can be satisfied by a JVM system property as well as an environment variable. Guard on both so this test doesn’t fail when the log level is set via -DTEXERA_SERVICE_LOG_LEVEL. ########## common/config/src/test/scala/org/apache/texera/common/config/UdfConfigSpec.scala: ########## @@ -0,0 +1,49 @@ +/* + * 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.texera.common.config + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Spec for [[UdfConfig]]. Reading each value forces resolution from udf.conf, so a renamed or + * mistyped key surfaces here as a ConfigException. Every value carries a `${?ENV}` override, so + * exact-value assertions are guarded on the override being absent from env and system properties. + */ +class UdfConfigSpec extends AnyFlatSpec with Matchers { + + private def ifUnset(name: String)(assertion: => Any): Unit = + if (!sys.env.contains(name) && !sys.props.contains(name)) assertion + + "UdfConfig" should "resolve the python and R defaults from udf.conf" in { + ifUnset("UDF_PYTHON_PATH")(UdfConfig.pythonPath shouldBe "") + ifUnset("UDF_R_PATH")(UdfConfig.rPath shouldBe "") + ifUnset("UDF_PYTHON_LOG_STREAMHANDLER_LEVEL")( + UdfConfig.pythonLogStreamHandlerLevel shouldBe "INFO" + ) + ifUnset("UDF_PYTHON_LOG_FILEHANDLER_DIR")(UdfConfig.pythonLogFileHandlerDir shouldBe "/tmp/") + ifUnset("UDF_PYTHON_LOG_FILEHANDLER_LEVEL")(UdfConfig.pythonLogFileHandlerLevel shouldBe "INFO") + } + + it should "resolve the non-empty log format strings" in { + UdfConfig.pythonLogStreamHandlerFormat should not be empty + UdfConfig.pythonLogFileHandlerFormat should not be empty + } Review Comment: These assertions can become flaky when UDF_PYTHON_LOG_*_FORMAT is set (udf.conf supports overrides). Guard them the same way as the other exact-value assertions so environments with custom formats don’t fail the tests. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
