You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "aglinxinyuan (via GitHub)" <gi...@apache.org> on 2026/06/16 22:39:37 UTC
[PR] test(workflow-core): add unit test coverage for VirtualCollection + ReadonlyVirtualDocument [texera]
aglinxinyuan opened a new pull request, #5740:
URL: https://github.com/apache/texera/pull/5740
### What changes were proposed in this PR?
Pin the abstract-base / trait contracts for two storage-model classes in `common/workflow-core/core/storage/model/`. Tested via minimal in-memory test-only concrete subclasses (no temp-file harness needed). No production-code changes.
| Spec | Source class | Tests |
| --- | --- | --- |
| `VirtualCollectionSpec` | `VirtualCollection` (abstract class) | 7 |
| `ReadonlyVirtualDocumentSpec` | `ReadonlyVirtualDocument[T]` (trait) | 11 |
Both spec files follow the `<srcClassName>Spec.scala` one-to-one convention.
**Behavior pinned — `VirtualCollection`**
| Surface | Contract |
| --- | --- |
| Abstract members (`getURI` / `getDocuments` / `getDocument` / `remove`) | pinned via a minimal `LinkedHashMap`-backed test-only subclass |
| `getDocuments` after two `addChild` calls | yields both documents in insertion order (URI sequence pinned to catch a `HashMap`-backed regression) |
| `getDocument(name)` for a registered name | returns the same `VirtualDocument[_]` reference (no copy) |
| `getDocument(name)` for an unregistered name | impl-defined throw (stub raises `NoSuchElementException`) |
| `remove()` | clears the child set (impl-defined side effect; mirrors production "delete folder" semantics) |
| Pattern matching | `case _: VirtualCollection` matches a concrete impl; does NOT match an unrelated type (`String`) |
**Behavior pinned — `ReadonlyVirtualDocument[T]`**
| Surface | Contract |
| --- | --- |
| Eight abstract methods (`getURI` / `getItem` / `get` / `getRange` / `getAfter` / `getCount` / `asInputStream` / `asFile`) | each is observable through a stub concrete subclass returning sentinel values |
| `getRange(from, until)` | half-open interval — `until` is exclusive |
| `getRange` call-site | `columns: Option[Seq[String]] = None` default lets the third argument be omitted |
| `getAfter(offset)` | skips the item AT `offset` and yields every strictly-later item |
| `asInputStream()` | non-null `InputStream` reading the impl-supplied bytes (read-1 / read-2 / EOF round-trip) |
| `asFile()` | non-null `File` |
| `getCount` | typed as `Long` (compile-time enforced) |
| Type parameter `T` | preserved across `getItem` / `get` / `getRange` / `getAfter` (verified with `T = Int`) |
| Pattern matching | `case _: ReadonlyVirtualDocument[_]` matches a concrete impl |
### Notes
`ReadonlyLocalFileDocument` (the concrete `private[storage]` impl of `ReadonlyVirtualDocument[Nothing]`) is **deliberately out of scope** for this PR — an existing spec at `amber/src/test/scala/org/apache/texera/workflow/common/storage/ReadonlyLocalFileDocumentSpec.scala` already pins its URI / `asInputStream` round-trip / `asFile` / five `NotImplementedError` cases via the `DocumentFactory.openReadonlyDocument(URI)` factory. Adding a parallel spec here would be the duplicate-spec failure mode the spec-filename convention is designed to prevent.
### Any related issues, documentation, discussions?
Closes #5736.
### How was this PR tested?
Pure unit-test additions; verified locally with:
- `sbt "WorkflowCore/testOnly org.apache.texera.amber.core.storage.model.VirtualCollectionSpec org.apache.texera.amber.core.storage.model.ReadonlyVirtualDocumentSpec"` — 18 tests, all green
- `sbt scalafmtCheckAll` — clean
- CI to confirm
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.7 [1M context])
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(workflow-core): add unit test coverage for VirtualCollection + ReadonlyVirtualDocument [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5740:
URL: https://github.com/apache/texera/pull/5740#issuecomment-4724247034
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 2 better · 🔴 2 worse · ⚪ 11 noise (<±5%) · 0 without baseline
> Compared against main `cb30970` benchmarked on **this same runner**, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.
[Dashboard](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/27652829043)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| 🔴 | bs=10 sw=10 sl=64 | 438 | 0.267 | 21,111/31,049/31,049 us | 🔴 -6.0% / 🟢 -11.2% |
| 🟢 | bs=100 sw=10 sl=64 | 951 | 0.58 | 105,836/117,758/117,758 us | 🟢 -11.4% / 🟢 -15.8% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,083 | 0.661 | 923,764/972,001/972,001 us | ⚪ within ±5% / 🟢 -5.0% |
<details><summary>Baseline details</summary>
Latest main `cb30970` from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 438 tuples/sec | 465 tuples/sec | 410.82 tuples/sec | -5.8% | +6.6% |
| bs=10 sw=10 sl=64 | MB/s | 0.267 MB/s | 0.284 MB/s | 0.251 MB/s | -6.0% | +6.5% |
| bs=10 sw=10 sl=64 | p50 | 21,111 us | 20,298 us | 23,785 us | +4.0% | -11.2% |
| bs=10 sw=10 sl=64 | p95 | 31,049 us | 32,601 us | 34,980 us | -4.8% | -11.2% |
| bs=10 sw=10 sl=64 | p99 | 31,049 us | 32,601 us | 34,980 us | -4.8% | -11.2% |
| bs=100 sw=10 sl=64 | throughput | 951 tuples/sec | 962 tuples/sec | 891.94 tuples/sec | -1.1% | +6.6% |
| bs=100 sw=10 sl=64 | MB/s | 0.58 MB/s | 0.587 MB/s | 0.544 MB/s | -1.2% | +6.5% |
| bs=100 sw=10 sl=64 | p50 | 105,836 us | 101,610 us | 112,277 us | +4.2% | -5.7% |
| bs=100 sw=10 sl=64 | p95 | 117,758 us | 132,926 us | 139,802 us | -11.4% | -15.8% |
| bs=100 sw=10 sl=64 | p99 | 117,758 us | 132,926 us | 139,802 us | -11.4% | -15.8% |
| bs=1000 sw=10 sl=64 | throughput | 1,083 tuples/sec | 1,109 tuples/sec | 1,041 tuples/sec | -2.3% | +4.0% |
| bs=1000 sw=10 sl=64 | MB/s | 0.661 MB/s | 0.677 MB/s | 0.635 MB/s | -2.4% | +4.0% |
| bs=1000 sw=10 sl=64 | p50 | 923,764 us | 902,549 us | 972,714 us | +2.4% | -5.0% |
| bs=1000 sw=10 sl=64 | p95 | 972,001 us | 955,086 us | 1,023,057 us | +1.8% | -5.0% |
| bs=1000 sw=10 sl=64 | p99 | 972,001 us | 955,086 us | 1,023,057 us | +1.8% | -5.0% |
</details>
<details><summary>Raw CSV</summary>
```csv
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,456.52,200,128000,438,0.267,21111.07,31049.12,31049.12
1,100,10,64,20,2103.35,2000,1280000,951,0.580,105836.02,117757.64,117757.64
2,1000,10,64,20,18474.11,20000,12800000,1083,0.661,923764.03,972000.81,972000.81
```
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(workflow-core): add unit test coverage for VirtualCollection + ReadonlyVirtualDocument [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5740:
URL: https://github.com/apache/texera/pull/5740#issuecomment-4724237001
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/5740?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 53.19%. Comparing base ([`cb30970`](https://app.codecov.io/gh/apache/texera/commit/cb30970bcfb915a19338d8a49e3cad3bd37796b8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`965e725`](https://app.codecov.io/gh/apache/texera/commit/965e725a63df779ee8d29c10b4289a275c045105?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #5740 +/- ##
=========================================
Coverage 53.19% 53.19%
Complexity 2656 2656
=========================================
Files 1094 1094
Lines 42363 42363
Branches 4556 4556
=========================================
Hits 22535 22535
Misses 18499 18499
Partials 1329 1329
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.44% <ø> (ø)` | | |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.36% <ø> (ø)` | | Carriedforward from [cb30970](https://app.codecov.io/gh/apache/texera/commit/cb30970bcfb915a19338d8a49e3cad3bd37796b8?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.49% <ø> (ø)` | | Carriedforward from [cb30970](https://app.codecov.io/gh/apache/texera/commit/cb30970bcfb915a19338d8a49e3cad3bd37796b8?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `1.65% <ø> (ø)` | | |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.71% <ø> (ø)` | | |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.06% <ø> (ø)` | | Carriedforward from [cb30970](https://app.codecov.io/gh/apache/texera/commit/cb30970bcfb915a19338d8a49e3cad3bd37796b8?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.02% <ø> (ø)` | | Carriedforward from [cb30970](https://app.codecov.io/gh/apache/texera/commit/cb30970bcfb915a19338d8a49e3cad3bd37796b8?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.13% <ø> (ø)` | | Carriedforward from [cb30970](https://app.codecov.io/gh/apache/texera/commit/cb30970bcfb915a19338d8a49e3cad3bd37796b8?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [python](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.80% <ø> (ø)` | | Carriedforward from [cb30970](https://app.codecov.io/gh/apache/texera/commit/cb30970bcfb915a19338d8a49e3cad3bd37796b8?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/5740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.69% <ø> (ø)` | | Carriedforward from [cb30970](https://app.codecov.io/gh/apache/texera/commit/cb30970bcfb915a19338d8a49e3cad3bd37796b8?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/5740?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(workflow-core): add unit test coverage for VirtualCollection + ReadonlyVirtualDocument [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5740:
URL: https://github.com/apache/texera/pull/5740#discussion_r3424446829
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/ReadonlyVirtualDocumentSpec.scala:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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)
+ override def asFile(): File = new File("/tmp/stub-int")
+ }
Review Comment:
Hard-coding a Unix `/tmp/...` path makes this test less portable (e.g., Windows), even though it only constructs a `File` object. Prefer using `java.io.tmpdir` to build a temp-path in an OS-independent way.
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/VirtualCollectionSpec.scala:
##########
@@ -0,0 +1,142 @@
+/*
+ * 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)
+
+ // ---------------------------------------------------------------------------
+ // Trait declares four abstract methods — pinned via concrete subclass
+ // ---------------------------------------------------------------------------
Review Comment:
This comment refers to `VirtualCollection` as a trait, but in production it’s an `abstract class` (`VirtualCollection.scala`). Keeping the wording accurate helps the spec serve as a contract reference.
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/ReadonlyVirtualDocumentSpec.scala:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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)
+ override def asFile(): File = new File("/tmp/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 {
+ val d = 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 {
+ val d = new StubReadonlyIntDoc(IndexedSeq(0, 1, 2))
+ // Call-site with all three positional args.
+ assert(d.getRange(0, 2, columns = Some(Seq("c"))).toList == List(0, 1))
+ // Call-site that omits the third arg — must compile via default.
+ assert(d.getRange(0, 2).toList == List(0, 1))
+ }
Review Comment:
Same issue as above: `getRange(0, 2)` is invoked with the 3rd argument omitted, but `d` is inferred as `StubReadonlyIntDoc`, so the default parameter from `ReadonlyVirtualDocument` won’t be applied at the call site. Type `d` as `ReadonlyVirtualDocument[Int]` to make the default-arg contract compile-time enforced.
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/ReadonlyVirtualDocumentSpec.scala:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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)
+ override def asFile(): File = new File("/tmp/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 {
+ val d = new StubReadonlyIntDoc(IndexedSeq(0, 1, 2, 3, 4))
+ assert(d.getRange(1, 4).toList == List(1, 2, 3))
+ }
Review Comment:
`ReadonlyVirtualDocument.getRange` has a default third parameter, but this test calls `getRange(1, 4)` on a value whose static type is `StubReadonlyIntDoc`. In Scala, default parameters are resolved from the static type, so this call won’t compile unless `d` is typed as `ReadonlyVirtualDocument[Int]` (or the stub defines its own default/overload).
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(workflow-core): add unit test coverage for VirtualCollection + ReadonlyVirtualDocument [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on code in PR #5740:
URL: https://github.com/apache/texera/pull/5740#discussion_r3425802600
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/ReadonlyVirtualDocumentSpec.scala:
##########
@@ -0,0 +1,163 @@
+/*
+ * 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 the optional `columns` argument and resolve its default (None) when omitted at the call site" in {
+ // The `columns` parameter exists so impls can project a subset of
+ // columns — `Some(...)` can LEGITIMATELY change the result on
+ // column-aware impls (e.g. iceberg-backed documents). What this
+ // case pins is the *call-site* contract: the third argument can
+ // be supplied or omitted, and the default is `None`.
+ //
+ // The stub deliberately ignores `columns` so the [from, until)
+ // slice is independent of the column selection — that lets us
+ // assert the call-site shape without taking a position on how
+ // impls should interpret `columns`.
+ val d: ReadonlyVirtualDocument[Int] = new StubReadonlyIntDoc(IndexedSeq(0, 1, 2))
+ // Call-site with all three positional args.
+ assert(d.getRange(0, 2, columns = Some(Seq("c"))).toList == List(0, 1))
+ // Call-site that omits the third arg — resolved via the trait's default.
+ assert(d.getRange(0, 2).toList == List(0, 1))
+ }
+
+ "ReadonlyVirtualDocument.getAfter" should
+ "skip the item at index `offset` and yield every item strictly after it" in {
+ val d = new StubReadonlyIntDoc(IndexedSeq(0, 1, 2, 3, 4))
+ assert(d.getAfter(1).toList == List(2, 3, 4))
+ }
+
+ "ReadonlyVirtualDocument.getCount" should "return the item count as a Long" in {
+ val d = new StubReadonlyIntDoc(IndexedSeq(0, 1, 2, 3, 4))
+ val count: Long = d.getCount
+ assert(count == 5L)
+ }
+
+ "ReadonlyVirtualDocument.asInputStream" should
+ "return a non-null InputStream that reads the impl-supplied bytes" in {
+ val d = new StubReadonlyIntDoc(IndexedSeq(7, 8))
+ val stream = d.asInputStream()
+ try {
+ assert(stream.read() == 7)
+ assert(stream.read() == 8)
+ assert(stream.read() == -1) // EOF
+ } finally {
+ stream.close()
+ }
+ }
+
+ "ReadonlyVirtualDocument.asFile" should "return a non-null File from the impl" in {
+ val d = new StubReadonlyIntDoc(IndexedSeq.empty)
+ val file = d.asFile()
+ assert(file != null)
+ assert(file.getPath != "")
+ }
+
+ // ---------------------------------------------------------------------------
+ // Type parameter — T is preserved across accessors
+ // ---------------------------------------------------------------------------
+
+ "ReadonlyVirtualDocument[Int]" should
+ "preserve the type parameter on every accessor (compile-time enforced)" in {
+ val d: ReadonlyVirtualDocument[Int] = new StubReadonlyIntDoc(IndexedSeq(1))
+ val item: Int = d.getItem(0)
+ val iter: Iterator[Int] = d.get()
+ val range: Iterator[Int] = d.getRange(0, 1)
+ val after: Iterator[Int] = d.getAfter(-1)
+ assert(item == 1)
+ assert(iter.toList == List(1))
+ assert(range.toList == List(1))
+ assert(after.toList == List(1))
+ }
Review Comment:
Fixed in ac8fa69157 — switched to a two-item fixture and `getAfter(0)` (which yields the second item, a documented non-negative offset). The case stays within the 0-based index range the trait docs describe.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(workflow-core): add unit test coverage for VirtualCollection + ReadonlyVirtualDocument [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(workflow-core): add unit test coverage for VirtualCollection + ReadonlyVirtualDocument [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on code in PR #5740:
URL: https://github.com/apache/texera/pull/5740#discussion_r3424478109
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/ReadonlyVirtualDocumentSpec.scala:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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)
+ override def asFile(): File = new File("/tmp/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 {
+ val d = new StubReadonlyIntDoc(IndexedSeq(0, 1, 2, 3, 4))
+ assert(d.getRange(1, 4).toList == List(1, 2, 3))
+ }
Review Comment:
Fixed in 154bbdc7d4 — the `getRange(1, 4)` case now types the value as `ReadonlyVirtualDocument[Int]`, so the default-arg contract is resolved through the trait at the call site (Scala resolves default parameters from the static type).
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/ReadonlyVirtualDocumentSpec.scala:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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)
+ override def asFile(): File = new File("/tmp/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 {
+ val d = 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 {
+ val d = new StubReadonlyIntDoc(IndexedSeq(0, 1, 2))
+ // Call-site with all three positional args.
+ assert(d.getRange(0, 2, columns = Some(Seq("c"))).toList == List(0, 1))
+ // Call-site that omits the third arg — must compile via default.
+ assert(d.getRange(0, 2).toList == List(0, 1))
+ }
Review Comment:
Fixed in 154bbdc7d4 — same approach: `d` is now declared as `ReadonlyVirtualDocument[Int]` so the `getRange(0, 2)` call resolves `columns: Option[…] = None` through the trait at compile time.
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/ReadonlyVirtualDocumentSpec.scala:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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)
+ override def asFile(): File = new File("/tmp/stub-int")
+ }
Review Comment:
Fixed in 154bbdc7d4 — replaced the hardcoded `/tmp/stub-int` path with `new File(System.getProperty("java.io.tmpdir"), "stub-int")` so the stub is OS-portable (Windows / macOS / Linux).
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/VirtualCollectionSpec.scala:
##########
@@ -0,0 +1,142 @@
+/*
+ * 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)
+
+ // ---------------------------------------------------------------------------
+ // Trait declares four abstract methods — pinned via concrete subclass
+ // ---------------------------------------------------------------------------
Review Comment:
Fixed in 154bbdc7d4 — replaced every "trait" reference with "abstract class" (in the section divider, the `getDocument` comment, and the type-pattern case-name + comment). VirtualCollection is declared `abstract class`, not `trait`.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(workflow-core): add unit test coverage for VirtualCollection + ReadonlyVirtualDocument [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(workflow-core): add unit test coverage for VirtualCollection + ReadonlyVirtualDocument [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan merged PR #5740:
URL: https://github.com/apache/texera/pull/5740
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(workflow-core): add unit test coverage for VirtualCollection + ReadonlyVirtualDocument [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5740:
URL: https://github.com/apache/texera/pull/5740#discussion_r3424796987
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/ReadonlyVirtualDocumentSpec.scala:
##########
@@ -0,0 +1,163 @@
+/*
+ * 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 the optional `columns` argument and resolve its default (None) when omitted at the call site" in {
+ // The `columns` parameter exists so impls can project a subset of
+ // columns — `Some(...)` can LEGITIMATELY change the result on
+ // column-aware impls (e.g. iceberg-backed documents). What this
+ // case pins is the *call-site* contract: the third argument can
+ // be supplied or omitted, and the default is `None`.
+ //
+ // The stub deliberately ignores `columns` so the [from, until)
+ // slice is independent of the column selection — that lets us
+ // assert the call-site shape without taking a position on how
+ // impls should interpret `columns`.
+ val d: ReadonlyVirtualDocument[Int] = new StubReadonlyIntDoc(IndexedSeq(0, 1, 2))
+ // Call-site with all three positional args.
+ assert(d.getRange(0, 2, columns = Some(Seq("c"))).toList == List(0, 1))
+ // Call-site that omits the third arg — resolved via the trait's default.
+ assert(d.getRange(0, 2).toList == List(0, 1))
+ }
+
+ "ReadonlyVirtualDocument.getAfter" should
+ "skip the item at index `offset` and yield every item strictly after it" in {
+ val d = new StubReadonlyIntDoc(IndexedSeq(0, 1, 2, 3, 4))
+ assert(d.getAfter(1).toList == List(2, 3, 4))
+ }
+
+ "ReadonlyVirtualDocument.getCount" should "return the item count as a Long" in {
+ val d = new StubReadonlyIntDoc(IndexedSeq(0, 1, 2, 3, 4))
+ val count: Long = d.getCount
+ assert(count == 5L)
+ }
+
+ "ReadonlyVirtualDocument.asInputStream" should
+ "return a non-null InputStream that reads the impl-supplied bytes" in {
+ val d = new StubReadonlyIntDoc(IndexedSeq(7, 8))
+ val stream = d.asInputStream()
+ try {
+ assert(stream.read() == 7)
+ assert(stream.read() == 8)
+ assert(stream.read() == -1) // EOF
+ } finally {
+ stream.close()
+ }
+ }
+
+ "ReadonlyVirtualDocument.asFile" should "return a non-null File from the impl" in {
+ val d = new StubReadonlyIntDoc(IndexedSeq.empty)
+ val file = d.asFile()
+ assert(file != null)
+ assert(file.getPath != "")
+ }
+
+ // ---------------------------------------------------------------------------
+ // Type parameter — T is preserved across accessors
+ // ---------------------------------------------------------------------------
+
+ "ReadonlyVirtualDocument[Int]" should
+ "preserve the type parameter on every accessor (compile-time enforced)" in {
+ val d: ReadonlyVirtualDocument[Int] = new StubReadonlyIntDoc(IndexedSeq(1))
+ val item: Int = d.getItem(0)
+ val iter: Iterator[Int] = d.get()
+ val range: Iterator[Int] = d.getRange(0, 1)
+ val after: Iterator[Int] = d.getAfter(-1)
+ assert(item == 1)
+ assert(iter.toList == List(1))
+ assert(range.toList == List(1))
+ assert(after.toList == List(1))
+ }
Review Comment:
In the type-parameter preservation test, using `getAfter(-1)` implies negative offsets are part of the contract. The trait docs describe `offset` as an index (0-based), so it’s clearer to use a non-negative offset (e.g., two items + `getAfter(0)` should yield the second item).
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org