aglinxinyuan commented on code in PR #7018: URL: https://github.com/apache/texera/pull/7018#discussion_r3671322909
########## 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: Good catch — those assertions were environment-dependent. The spec now uses a `withNullGit` helper that reflectively forces OPVersion's private static `git` field to null for the duration of each test and restores the original in a `finally` (so no other suite in the same JVM is affected). The N/A fallback and the memoization contract are now asserted against one known handle state instead of depending on whether the checkout is a worktree or a plain clone. OPVersion.java is unchanged. ########## 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: Right — and that null-return path is real: the GitAPIException catch never populates `opMap`, so `opMap.get(...)` can return null. Rather than assert against that ambiguity, every test now runs with the git handle pinned to null (see the sibling thread), which makes the fallback deterministic; I documented the un-populated-on-GitAPIException behavior in the spec's scaladoc rather than papering over it. Changing OPVersion to store a fallback on GitAPIException seems reasonable but is a source change beyond this test PR — happy to open a follow-up if you'd like it. -- 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]
