aglinxinyuan commented on code in PR #5740: URL: https://github.com/apache/texera/pull/5740#discussion_r3424516854
########## common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/VirtualCollectionSpec.scala: ########## @@ -0,0 +1,145 @@ +/* + * 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.core.storage.model + +import org.scalatest.flatspec.AnyFlatSpec + +import java.net.URI +import scala.collection.mutable + +class VirtualCollectionSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Test harness — a minimal in-memory concrete impl exercises every + // abstract method (getURI / getDocuments / getDocument / remove). + // + // The contained `VirtualDocument`s are stubbed with the smallest + // concrete impl: `clear()` and `getURI` are the only abstract members + // not given a default by the base class. + // --------------------------------------------------------------------------- + + private class StubDocument(uriValue: URI) extends VirtualDocument[Nothing] { + override def getURI: URI = uriValue + override def clear(): Unit = () + } + + private class StubCollection(uriValue: URI) extends VirtualCollection { + private val children = mutable.LinkedHashMap.empty[String, VirtualDocument[_]] + private var removed = false + + def addChild(name: String, doc: VirtualDocument[_]): Unit = children(name) = doc + def wasRemoved: Boolean = removed + + override def getURI: URI = uriValue + override def getDocuments: List[VirtualDocument[_]] = children.values.toList + override def getDocument(name: String): VirtualDocument[_] = + children.getOrElse(name, throw new NoSuchElementException(name)) + override def remove(): Unit = { + children.clear() + removed = true + } + } + + private def uri(s: String): URI = new URI(s) + + // --------------------------------------------------------------------------- + // Abstract class declares four abstract methods — pinned via a + // concrete subclass. (VirtualCollection is an `abstract class`, not a + // trait — see `VirtualCollection.scala`.) + // --------------------------------------------------------------------------- + + "VirtualCollection (concrete subclass)" should "delegate getURI to the implementation" in { + val c = new StubCollection(uri("file:///tmp/coll")) + assert(c.getURI == uri("file:///tmp/coll")) + } + + it should "expose getDocuments as the list of registered child documents (insertion order)" in { Review Comment: Fixed in 494d234773 — renamed the case from "insertion order" to "membership" and switched the assertion to `docs.map(_.getURI).toSet == Set(docA.getURI, docB.getURI)`. The abstract class does not document an ordering guarantee, so over-constraining the order would be brittle for legitimate future impls. ########## common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/VirtualCollectionSpec.scala: ########## @@ -0,0 +1,145 @@ +/* + * 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.core.storage.model + +import org.scalatest.flatspec.AnyFlatSpec + +import java.net.URI +import scala.collection.mutable + +class VirtualCollectionSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Test harness — a minimal in-memory concrete impl exercises every + // abstract method (getURI / getDocuments / getDocument / remove). + // + // The contained `VirtualDocument`s are stubbed with the smallest + // concrete impl: `clear()` and `getURI` are the only abstract members + // not given a default by the base class. + // --------------------------------------------------------------------------- + + private class StubDocument(uriValue: URI) extends VirtualDocument[Nothing] { + override def getURI: URI = uriValue + override def clear(): Unit = () + } + + private class StubCollection(uriValue: URI) extends VirtualCollection { + private val children = mutable.LinkedHashMap.empty[String, VirtualDocument[_]] + private var removed = false + + def addChild(name: String, doc: VirtualDocument[_]): Unit = children(name) = doc + def wasRemoved: Boolean = removed + + override def getURI: URI = uriValue + override def getDocuments: List[VirtualDocument[_]] = children.values.toList + override def getDocument(name: String): VirtualDocument[_] = + children.getOrElse(name, throw new NoSuchElementException(name)) + override def remove(): Unit = { + children.clear() + removed = true + } + } + + private def uri(s: String): URI = new URI(s) + + // --------------------------------------------------------------------------- + // Abstract class declares four abstract methods — pinned via a + // concrete subclass. (VirtualCollection is an `abstract class`, not a + // trait — see `VirtualCollection.scala`.) + // --------------------------------------------------------------------------- + + "VirtualCollection (concrete subclass)" should "delegate getURI to the implementation" in { + val c = new StubCollection(uri("file:///tmp/coll")) + assert(c.getURI == uri("file:///tmp/coll")) + } + + it should "expose getDocuments as the list of registered child documents (insertion order)" in { + val c = new StubCollection(uri("file:///coll")) + assert(c.getDocuments.isEmpty) + val docA = new StubDocument(uri("file:///coll/a")) + val docB = new StubDocument(uri("file:///coll/b")) + c.addChild("a", docA) + c.addChild("b", docB) + val docs = c.getDocuments + assert(docs.size == 2) + // LinkedHashMap preserves insertion order; pin the URI sequence so + // a regression to HashMap-backed storage would surface here. + assert(docs.map(_.getURI) == List(docA.getURI, docB.getURI)) Review Comment: Fixed in 494d234773 — same case; assertion is now on the URI set, not the list sequence. ########## common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/VirtualCollectionSpec.scala: ########## @@ -0,0 +1,145 @@ +/* + * 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.core.storage.model + +import org.scalatest.flatspec.AnyFlatSpec + +import java.net.URI +import scala.collection.mutable + +class VirtualCollectionSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Test harness — a minimal in-memory concrete impl exercises every + // abstract method (getURI / getDocuments / getDocument / remove). + // + // The contained `VirtualDocument`s are stubbed with the smallest + // concrete impl: `clear()` and `getURI` are the only abstract members + // not given a default by the base class. + // --------------------------------------------------------------------------- + + private class StubDocument(uriValue: URI) extends VirtualDocument[Nothing] { + override def getURI: URI = uriValue + override def clear(): Unit = () + } + + private class StubCollection(uriValue: URI) extends VirtualCollection { + private val children = mutable.LinkedHashMap.empty[String, VirtualDocument[_]] + private var removed = false + + def addChild(name: String, doc: VirtualDocument[_]): Unit = children(name) = doc + def wasRemoved: Boolean = removed + + override def getURI: URI = uriValue + override def getDocuments: List[VirtualDocument[_]] = children.values.toList + override def getDocument(name: String): VirtualDocument[_] = + children.getOrElse(name, throw new NoSuchElementException(name)) + override def remove(): Unit = { + children.clear() + removed = true + } + } + + private def uri(s: String): URI = new URI(s) + + // --------------------------------------------------------------------------- + // Abstract class declares four abstract methods — pinned via a + // concrete subclass. (VirtualCollection is an `abstract class`, not a + // trait — see `VirtualCollection.scala`.) + // --------------------------------------------------------------------------- + + "VirtualCollection (concrete subclass)" should "delegate getURI to the implementation" in { + val c = new StubCollection(uri("file:///tmp/coll")) + assert(c.getURI == uri("file:///tmp/coll")) + } + + it should "expose getDocuments as the list of registered child documents (insertion order)" in { + val c = new StubCollection(uri("file:///coll")) + assert(c.getDocuments.isEmpty) + val docA = new StubDocument(uri("file:///coll/a")) + val docB = new StubDocument(uri("file:///coll/b")) + c.addChild("a", docA) + c.addChild("b", docB) + val docs = c.getDocuments + assert(docs.size == 2) + // LinkedHashMap preserves insertion order; pin the URI sequence so + // a regression to HashMap-backed storage would surface here. + assert(docs.map(_.getURI) == List(docA.getURI, docB.getURI)) + } + + it should "look up a child by name via getDocument" in { + val c = new StubCollection(uri("file:///coll")) + val doc = new StubDocument(uri("file:///coll/only")) + c.addChild("only", doc) + // Pin that the same reference is returned (no copy). + assert(c.getDocument("only") eq doc) + } + + it should "let getDocument signal a missing child (the spec leaves that to impls)" in { + // The abstract class declares `getDocument(name): VirtualDocument[_]` + // with no exception specification — impls choose how to signal a + // missing child. The stub raises NoSuchElementException; pin that + // behavior. + val c = new StubCollection(uri("file:///coll")) + intercept[NoSuchElementException] { Review Comment: Fixed in 494d234773 — stopped pinning `NoSuchElementException`. The case now uses `scala.util.Try(c.getDocument(...))` and asserts only that the lookup signals FAILURE (`isFailure`), not the failure type. The abstract class deliberately leaves the exception type impl-defined, so the test no longer reads like an implicit contract on every future impl. ########## common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/ReadonlyVirtualDocumentSpec.scala: ########## @@ -0,0 +1,152 @@ +/* + * 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.core.storage.model + +import org.scalatest.flatspec.AnyFlatSpec + +import java.io.{ByteArrayInputStream, File, InputStream} +import java.net.URI + +class ReadonlyVirtualDocumentSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Stub impl — provides one sentinel value per accessor so each abstract + // method can be exercised in isolation, plus an int-typed variant so + // the type parameter `T` is observed end-to-end. + // --------------------------------------------------------------------------- + + private class StubReadonlyIntDoc(items: IndexedSeq[Int]) extends ReadonlyVirtualDocument[Int] { + override def getURI: URI = new URI("file:///stub/int") + override def getItem(i: Int): Int = items(i) + override def get(): Iterator[Int] = items.iterator + override def getRange(from: Int, until: Int, columns: Option[Seq[String]]): Iterator[Int] = + items.slice(from, until).iterator + override def getAfter(offset: Int): Iterator[Int] = items.drop(offset + 1).iterator + override def getCount: Long = items.size.toLong + override def asInputStream(): InputStream = + new ByteArrayInputStream(items.map(_.toByte).toArray) + // Use the OS-portable temp directory rather than a hardcoded + // `/tmp/...` path so the spec also runs on Windows. + override def asFile(): File = + new File(System.getProperty("java.io.tmpdir"), "stub-int") + } + + // --------------------------------------------------------------------------- + // Accessor surface — return verbatim + // --------------------------------------------------------------------------- + + "ReadonlyVirtualDocument.getURI" should "return the impl-supplied URI" in { + val d = new StubReadonlyIntDoc(IndexedSeq(1, 2, 3)) + assert(d.getURI == new URI("file:///stub/int")) + } + + "ReadonlyVirtualDocument.getItem" should "delegate to the impl's index lookup" in { + val d = new StubReadonlyIntDoc(IndexedSeq(10, 20, 30)) + assert(d.getItem(0) == 10) + assert(d.getItem(2) == 30) + } + + "ReadonlyVirtualDocument.get" should "iterate every item from the impl in order" in { + val d = new StubReadonlyIntDoc(IndexedSeq(7, 8, 9)) + assert(d.get().toList == List(7, 8, 9)) + } + + "ReadonlyVirtualDocument.getRange" should + "yield items in `[from, until)` (half-open interval — `until` is exclusive)" in { + // Type via the trait so the default-arg contract (`columns: Option[…] = None`) + // is resolved at the call site through the trait's signature, not the + // concrete subclass. Scala resolves default parameters from the + // STATIC type, so a `StubReadonlyIntDoc`-typed value without its own + // default would not get a default at the call site. + val d: ReadonlyVirtualDocument[Int] = + new StubReadonlyIntDoc(IndexedSeq(0, 1, 2, 3, 4)) + assert(d.getRange(1, 4).toList == List(1, 2, 3)) + } + + it should "accept an optional `columns` argument (default None) without changing the result" in { Review Comment: Fixed in 494d234773 — renamed the case to describe the actual contract: "accept the optional `columns` argument and resolve its default (None) when omitted at the call site". Comment now notes that `columns` LEGITIMATELY controls projection on column-aware impls, and that the stub deliberately ignores `columns` to keep the assertion column-independent. -- 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]
