Copilot commented on code in PR #5740: URL: https://github.com/apache/texera/pull/5740#discussion_r3424488919
########## 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: The `VirtualCollection` API/docs don’t specify that `getDocuments` must preserve insertion order. Encoding “(insertion order)” in the spec name makes this read like a required contract and can become a brittle constraint if implementations legitimately return a different (but still valid) order. ########## 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: The test text says the missing-child behavior is “impl-defined”, but the assertion pins a specific exception type. If the contract is intentionally unspecified, it’s better not to require `NoSuchElementException` here (otherwise it becomes an implied requirement). ########## 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: This assertion (and the preceding comment) requires insertion order even though `VirtualCollection.getDocuments` doesn’t document any ordering guarantee. If ordering is not part of the contract, assert membership rather than exact sequence to avoid over-constraining future implementations. ########## 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: `ReadonlyVirtualDocument.getRange` documents that `columns` controls projection (Some(...) can legitimately change results). The spec name “without changing the result” is misleading here; the key contract being tested is that the parameter exists and the default can be omitted at the call site. -- 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]
