Copilot commented on code in PR #7018: URL: https://github.com/apache/texera/pull/7018#discussion_r3671268038
########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/metadata/OPVersionSpec.scala: ########## @@ -0,0 +1,123 @@ +/* + * 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.amber.operator.metadata + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.util.UUID + +/** + * `OPVersion` resolves an operator's version from the git history of the file + * that defines it, memoizing the answer in a process-wide static map. + * + * Its git handle is opened once in a static initializer against `TEXERA_HOME` + * (defaulting to the working directory). Whether that succeeds depends entirely + * on how the tree was checked out — inside a `git worktree` the `.git` entry is + * a file rather than a directory and jgit raises `RepositoryNotFoundException`, + * leaving the handle null. This spec therefore asserts only behavior that holds + * either way: + * + * - a path with no commit history resolves to the `"N/A"` fallback (the handle + * is null and dereferencing it throws, or the log is empty and reading the + * first commit throws — both land in the same `NullPointerException` catch); + * - resolution is memoized per operator name and the path is ignored on a hit. + * + * Deliberately NOT covered: the success path that returns a real commit hash and + * the `GitAPIException` catch. Both require a specific, openable repository state + * that is not guaranteed for a test run. + */ +class OPVersionSpec extends AnyFlatSpec with Matchers { + + /** The cache is static and shared, so every test uses a name nothing else can collide with. */ + private def uniqueName(): String = s"OPVersionSpec-${UUID.randomUUID()}" + + private def uniqueMissingPath(): String = s"no/such/operator/path/${UUID.randomUUID()}" + + /** The private static memo table, so tests can seed it and clean up after themselves. */ + private def opMap: java.util.Map[String, String] = { + val field = classOf[OPVersion].getDeclaredField("opMap") + field.setAccessible(true) + field.get(null).asInstanceOf[java.util.Map[String, String]] + } + + private def withCleanCache[T](names: String*)(body: => T): T = + try body + finally names.foreach(opMap.remove) + Review Comment: The OPVersion tests currently assume missing/unknown paths resolve to "N/A". In OPVersion.java, only NullPointerException is caught; if git opens successfully, a path with no history is more likely to yield an empty iterator (NoSuchElementException) or other failures, which would make these tests flaky across different checkout types. Consider making the fallback path deterministic in the spec (e.g., temporarily nulling OPVersion's private static `git` field via reflection) before asserting "N/A" behavior. This issue also appears in the following locations of the same file: - line 65 - line 81 - line 113 ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/metadata/OPVersionSpec.scala: ########## @@ -0,0 +1,123 @@ +/* + * 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.amber.operator.metadata + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.util.UUID + +/** + * `OPVersion` resolves an operator's version from the git history of the file + * that defines it, memoizing the answer in a process-wide static map. + * + * Its git handle is opened once in a static initializer against `TEXERA_HOME` + * (defaulting to the working directory). Whether that succeeds depends entirely + * on how the tree was checked out — inside a `git worktree` the `.git` entry is + * a file rather than a directory and jgit raises `RepositoryNotFoundException`, + * leaving the handle null. This spec therefore asserts only behavior that holds + * either way: + * + * - a path with no commit history resolves to the `"N/A"` fallback (the handle + * is null and dereferencing it throws, or the log is empty and reading the + * first commit throws — both land in the same `NullPointerException` catch); + * - resolution is memoized per operator name and the path is ignored on a hit. + * + * Deliberately NOT covered: the success path that returns a real commit hash and + * the `GitAPIException` catch. Both require a specific, openable repository state + * that is not guaranteed for a test run. + */ +class OPVersionSpec extends AnyFlatSpec with Matchers { + + /** The cache is static and shared, so every test uses a name nothing else can collide with. */ + private def uniqueName(): String = s"OPVersionSpec-${UUID.randomUUID()}" + + private def uniqueMissingPath(): String = s"no/such/operator/path/${UUID.randomUUID()}" + + /** The private static memo table, so tests can seed it and clean up after themselves. */ + private def opMap: java.util.Map[String, String] = { + val field = classOf[OPVersion].getDeclaredField("opMap") + field.setAccessible(true) + field.get(null).asInstanceOf[java.util.Map[String, String]] + } + + private def withCleanCache[T](names: String*)(body: => T): T = + try body + finally names.foreach(opMap.remove) + + "OPVersion.getVersion" should "fall back to \"N/A\" for a path with no commit history" in { + val name = uniqueName() + withCleanCache(name) { + OPVersion.getVersion(name, uniqueMissingPath()) shouldBe "N/A" + } + } + + it should "never return null, whatever it resolves" in { + val name = uniqueName() + withCleanCache(name) { + OPVersion.getVersion(name, "common/workflow-operator/src/main/scala") should not be null + } + } Review Comment: `OPVersion.getVersion` can return null today: in OPVersion.java the GitAPIException catch prints a stack trace but does not populate `opMap`, so the subsequent `return opMap.get(operatorName)` may be null. To keep this test deterministic and aligned with current behavior, force the NPE fallback path (git == null) or change OPVersion to store a fallback value on GitAPIException too. -- 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]
