You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@texera.apache.org by gi...@apache.org on 2026/06/15 23:24:04 UTC

(texera) branch gh-readonly-queue/main/pr-5722-2b9add956c9e63c3c4f6e717221a0c5e33e54875 created (now 33903e1188)

This is an automated email from the ASF dual-hosted git repository.

github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-5722-2b9add956c9e63c3c4f6e717221a0c5e33e54875
in repository https://gitbox.apache.org/repos/asf/texera.git


      at 33903e1188 test(workflow-core): add dedicated GlobalPortIdentitySerde spec (#5722)

This branch includes the following new commits:

     new 33903e1188 test(workflow-core): add dedicated GlobalPortIdentitySerde spec (#5722)

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.



(texera) 01/01: test(workflow-core): add dedicated GlobalPortIdentitySerde spec (#5722)

Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-5722-2b9add956c9e63c3c4f6e717221a0c5e33e54875
in repository https://gitbox.apache.org/repos/asf/texera.git

commit 33903e11883389d1fee8bae134188aa407222130
Author: Eugene Gu <eu...@outlook.com>
AuthorDate: Mon Jun 15 16:19:26 2026 -0700

    test(workflow-core): add dedicated GlobalPortIdentitySerde spec (#5722)
    
    ### What changes were proposed in this PR?
    
    This PR adds a dedicated `GlobalPortIdentitySerdeSpec` for
    `GlobalPortIdentitySerde`, the helper that serializes/deserializes a
    `GlobalPortIdentity` to the compact, underscore-free string used in VFS
    URIs and file paths.
    
    `GlobalPortIdentitySerde` was previously exercised inside
    `PortIdentitySerdeSpec`, which mixed it with the unrelated
    `PortIdentity` Jackson key (de)serializer tests. Rather than duplicate
    that coverage, this PR:
    
    - **Moves** the `GlobalPortIdentitySerde` cases into their own file (the
    name issue #5717 asks for), so the two serde concerns are tested
    independently.
    - **Trims** `PortIdentitySerdeSpec` down to only the `PortIdentity`
    Jackson key (de)serialization tests, dropping the now-unused imports.
    - **Adds** seven edge cases that were previously unpinned (see below).
    
    There are **no production-code changes** and **no duplication** between
    the two specs.
    
    New edge cases added beyond the migrated coverage:
    
    1. **serialize/deserialize asymmetry** — the deserializer regex accepts
    underscores even though the serializer rejects them; characterized so a
    future tightening of the regex breaks the test on purpose.
    2. **unescaped comma** — a `logicalOpId` containing a `,` fails to
    round-trip, because the format does not escape its separator.
    3. **portId boundary** — `Int.MaxValue` round-trips, and an
    out-of-Int-range value (`9999999999`) is rejected with
    `NumberFormatException` on deserialize.
    4. **mixed-case booleans** — `True` / `FALSE` are accepted on
    deserialize, since Scala's `String.toBoolean` is case-insensitive.
    5. **`=` in a value round-trips** — only `,` is a sensitive separator;
    an embedded `=` is captured fine, the counterpart to the comma case.
    6. **empty `logicalOpId`** — serializes into `(logicalOpId=,...)` but
    the result can no longer be deserialized (`[^,]+` requires ≥1 char), the
    serialize side of the empty-field asymmetry.
    7. **require messages name the field** — the serialize-side guards
    identify the offending field (`logicalOpId` / `layerName` / `portId`) in
    their message, catching a regression that wires a check to the wrong
    field.
    
    | File | Change |
    | --- | --- |
    |
    `common/workflow-core/.../util/serde/GlobalPortIdentitySerdeSpec.scala`
    | **new** — 23 tests (`AnyFlatSpec with Matchers`) |
    | `common/workflow-core/.../util/serde/PortIdentitySerdeSpec.scala` |
    removed the migrated `GlobalPortIdentitySerde` block + now-unused
    imports (−189 lines); `PortIdentity` Jackson key (de)serialization tests
    kept intact |
    
    ### Any related issues, documentation, discussions?
    
    Closes #5717
    
    ### How was this PR tested?
    
    Test-only PR. All checks below were run locally from the repository root
    and pass:
    
    1. **Unit tests** — the new and modified specs:
       ```
    sbt "WorkflowCore/testOnly
    org.apache.texera.amber.util.serde.GlobalPortIdentitySerdeSpec
    org.apache.texera.amber.util.serde.PortIdentitySerdeSpec"
       ```
    Result: **31 tests, 0 failures** (1 `pendingUntilFixed`, pre-existing in
    `PortIdentitySerdeSpec`).
    
    2. **Lint (scalafix)** — `sbt "scalafixAll --check"` → success.
    
    3. **Format (scalafmt)** — `sbt scalafmtCheckAll` → success.
    
    ### Was this PR authored or co-authored using generative AI tooling?
    
    Co-authored by: Claude Opus 4.8
---
 ...pec.scala => GlobalPortIdentitySerdeSpec.scala} | 212 +++++++++------------
 .../amber/util/serde/PortIdentitySerdeSpec.scala   | 190 +-----------------
 2 files changed, 91 insertions(+), 311 deletions(-)

diff --git a/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentitySerdeSpec.scala b/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/GlobalPortIdentitySerdeSpec.scala
similarity index 56%
copy from common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentitySerdeSpec.scala
copy to common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/GlobalPortIdentitySerdeSpec.scala
index 89a815c39e..2dc455e32a 100644
--- a/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentitySerdeSpec.scala
+++ b/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/GlobalPortIdentitySerdeSpec.scala
@@ -21,15 +21,11 @@ package org.apache.texera.amber.util.serde
 
 import org.apache.texera.amber.core.virtualidentity.{OperatorIdentity, PhysicalOpIdentity}
 import org.apache.texera.amber.core.workflow.{GlobalPortIdentity, PortIdentity}
-import org.apache.texera.amber.util.JSONUtils.objectMapper
 import org.apache.texera.amber.util.serde.GlobalPortIdentitySerde.SerdeOps
 import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
 
-class PortIdentitySerdeSpec extends AnyFlatSpec {
-
-  // ---------------------------------------------------------------------------
-  // GlobalPortIdentitySerde
-  // ---------------------------------------------------------------------------
+class GlobalPortIdentitySerdeSpec extends AnyFlatSpec with Matchers {
 
   private def globalPort(
       logical: String = "op-A",
@@ -47,7 +43,7 @@ class PortIdentitySerdeSpec extends AnyFlatSpec {
   "GlobalPortIdentitySerde" should "round-trip a default GlobalPortIdentity through serializeAsString → deserializeFromString" in {
     val original = globalPort()
     val restored = GlobalPortIdentitySerde.deserializeFromString(original.serializeAsString)
-    assert(restored == original)
+    restored shouldBe original
   }
 
   it should "preserve all five fields independently across the round-trip" in {
@@ -69,7 +65,7 @@ class PortIdentitySerdeSpec extends AnyFlatSpec {
     cases.foreach { p =>
       val s = p.serializeAsString
       val restored = GlobalPortIdentitySerde.deserializeFromString(s)
-      assert(restored == p, s"round-trip mismatch for $p (serialized: $s)")
+      restored shouldBe p
     }
   }
 
@@ -77,20 +73,16 @@ class PortIdentitySerdeSpec extends AnyFlatSpec {
     // Pin the exact format. If this changes, callers reading existing
     // VFS URIs from disk will break — locking it down forces a deliberate
     // migration story.
-    assert(
-      globalPort().serializeAsString ==
-        "(logicalOpId=op-A,layerName=main,portId=0,isInternal=false,isInput=true)"
-    )
-    assert(
-      globalPort(
-        logical = "op-Z",
-        layer = "extra-layer",
-        portIdValue = 7,
-        internal = true,
-        input = false
-      ).serializeAsString ==
-        "(logicalOpId=op-Z,layerName=extra-layer,portId=7,isInternal=true,isInput=false)"
-    )
+    globalPort().serializeAsString shouldBe
+      "(logicalOpId=op-A,layerName=main,portId=0,isInternal=false,isInput=true)"
+    globalPort(
+      logical = "op-Z",
+      layer = "extra-layer",
+      portIdValue = 7,
+      internal = true,
+      input = false
+    ).serializeAsString shouldBe
+      "(logicalOpId=op-Z,layerName=extra-layer,portId=7,isInternal=true,isInput=false)"
   }
 
   it should "round-trip identifiers containing dashes and dots (regex non-comma matcher)" in {
@@ -100,7 +92,7 @@ class PortIdentitySerdeSpec extends AnyFlatSpec {
     // both; if the regex were ever tightened to alphanumerics only, this
     // would fail on purpose.
     val p = globalPort(logical = "my.op-with-dashes.v2", layer = "main-1")
-    assert(GlobalPortIdentitySerde.deserializeFromString(p.serializeAsString) == p)
+    GlobalPortIdentitySerde.deserializeFromString(p.serializeAsString) shouldBe p
   }
 
   it should "throw IllegalArgumentException when serializing a negative port id" in {
@@ -147,13 +139,6 @@ class PortIdentitySerdeSpec extends AnyFlatSpec {
     }
   }
 
-  it should "throw IllegalArgumentException on a completely malformed string" in {
-    val ex = intercept[IllegalArgumentException] {
-      GlobalPortIdentitySerde.deserializeFromString("not even close")
-    }
-    assert(ex.getMessage.contains("not even close"))
-  }
-
   it should "throw IllegalArgumentException when a required field is missing" in {
     // Drop isInput.
     val malformed = "(logicalOpId=op-A,layerName=main,portId=0,isInternal=false)"
@@ -173,7 +158,7 @@ class PortIdentitySerdeSpec extends AnyFlatSpec {
   }
 
   it should "throw IllegalArgumentException when a boolean field is non-boolean" in {
-    // `String.toBoolean` is strict: only \"true\" / \"false\" (case-insensitive)
+    // `String.toBoolean` is strict: only "true" / "false" (case-insensitive)
     // pass; anything else throws IllegalArgumentException.
     val malformed = "(logicalOpId=op-A,layerName=main,portId=0,isInternal=maybe,isInput=true)"
     intercept[IllegalArgumentException] {
@@ -181,18 +166,22 @@ class PortIdentitySerdeSpec extends AnyFlatSpec {
     }
   }
 
+  it should "throw IllegalArgumentException on a completely malformed string" in {
+    val ex = intercept[IllegalArgumentException] {
+      GlobalPortIdentitySerde.deserializeFromString("not even close")
+    }
+    ex.getMessage should include("not even close")
+  }
+
   it should "use no underscore in its own format characters (separators / keys)" in {
     // Pin the format-character invariant: the wrapping `(...)`, the field
     // separators `,`, the key=value separators, and the field NAMES
-    // themselves contain no underscore. Verify by building the format with
-    // empty-string-replacement values for every input field, so anything
-    // left in the output is purely from `serializeAsString`'s own format.
-    // (For the layerName field the empty-input variant is rejected by the
-    // deserializer regex; here we only check the SERIALIZED output, not the
-    // round-trip.)
+    // themselves contain no underscore. Verify by stripping the input
+    // field values, so anything left in the output is purely from
+    // `serializeAsString`'s own format.
     val s = globalPort(logical = "x", layer = "x").serializeAsString
     val formatChars = s.replace("x", "").replace("0", "").replace("false", "").replace("true", "")
-    assert(!formatChars.contains("_"), s"format characters must be underscore-free: $formatChars")
+    formatChars should not include "_"
   }
 
   it should "throw IllegalArgumentException when logicalOpId contains an underscore" in {
@@ -214,107 +203,86 @@ class PortIdentitySerdeSpec extends AnyFlatSpec {
   }
 
   // ---------------------------------------------------------------------------
-  // PortIdentityKeySerializer.portIdToString (companion, not the Jackson class)
-  // ---------------------------------------------------------------------------
-
-  "PortIdentityKeySerializer.portIdToString" should "format a PortIdentity as `id_internal`" in {
-    assert(PortIdentityKeySerializer.portIdToString(PortIdentity(0, internal = false)) == "0_false")
-    assert(PortIdentityKeySerializer.portIdToString(PortIdentity(7, internal = true)) == "7_true")
-  }
-
-  // ---------------------------------------------------------------------------
-  // PortIdentityKeySerializer + PortIdentityKeyDeserializer (Jackson wiring)
+  // Edge cases
   // ---------------------------------------------------------------------------
-  //
-  // These tests use the production `JSONUtils.objectMapper` directly so a
-  // regression in the singleton wiring (e.g. the module that registers the
-  // PortIdentity key (de)serializer being removed or reordered) surfaces
-  // here, not just on a freshly-constructed mapper.
-
-  "PortIdentity Jackson key (de)serialization" should "round-trip a Map[PortIdentity, String] via JSONUtils.objectMapper" in {
-    val original = Map(
-      PortIdentity(0, internal = false) -> "a",
-      PortIdentity(1, internal = true) -> "b"
-    )
-    val json = objectMapper.writeValueAsString(original)
-    // Verify the JSON keys match the documented `id_internal` format.
-    assert(json.contains("\"0_false\""))
-    assert(json.contains("\"1_true\""))
-    val tref = objectMapper.getTypeFactory
-      .constructMapType(classOf[java.util.HashMap[_, _]], classOf[PortIdentity], classOf[String])
-    val restored: java.util.Map[PortIdentity, String] = objectMapper.readValue(json, tref)
-    import scala.jdk.CollectionConverters._
-    assert(restored.asScala.toMap == original)
-  }
-
-  it should "round-trip an empty Map[PortIdentity, V] without invoking the (de)serializer" in {
-    val original = Map.empty[PortIdentity, String]
-    val json = objectMapper.writeValueAsString(original)
-    val tref = objectMapper.getTypeFactory
-      .constructMapType(classOf[java.util.HashMap[_, _]], classOf[PortIdentity], classOf[String])
-    val restored: java.util.Map[PortIdentity, String] = objectMapper.readValue(json, tref)
-    assert(restored.isEmpty)
-  }
 
-  "PortIdentityKeyDeserializer.deserializeKey" should "throw NumberFormatException for a non-integer id" in {
-    val d = new PortIdentityKeyDeserializer
-    intercept[NumberFormatException] {
-      d.deserializeKey("notAnInt_false", null)
-    }
+  it should "accept underscores on deserialize even though serialize rejects them (serialize/deserialize asymmetry)" in {
+    // The deserializer regex `[^,]+` does not reject underscores, so a
+    // hand-crafted string with underscored logicalOpId / layerName parses
+    // fine — unlike the serializer, which rejects them at the boundary.
+    // Characterize this asymmetry so a future tightening of the regex
+    // breaks this test deliberately.
+    val s = "(logicalOpId=op_A,layerName=main_layer,portId=0,isInternal=false,isInput=true)"
+    val restored = GlobalPortIdentitySerde.deserializeFromString(s)
+    restored.opId.logicalOpId.id shouldBe "op_A"
+    restored.opId.layerName shouldBe "main_layer"
   }
 
-  it should "throw IllegalArgumentException for a non-boolean internal flag" in {
-    val d = new PortIdentityKeyDeserializer
+  it should "fail to round-trip a logicalOpId containing a comma (separators are not escaped)" in {
+    // The format does not escape its `,` separator, so a logicalOpId with
+    // an embedded comma serializes into a string the deserializer can no
+    // longer parse back into the same value.
+    val s = globalPort(logical = "op,A").serializeAsString
     intercept[IllegalArgumentException] {
-      d.deserializeKey("0_notABool", null)
+      GlobalPortIdentitySerde.deserializeFromString(s)
     }
   }
 
-  it should "throw NumberFormatException when the underscore separator is missing and the whole string is non-numeric" in {
-    // `key.split("_")` on a separator-less non-numeric string yields a
-    // single-element array, and `parts(0).toInt` fires first → NFE.
-    val d = new PortIdentityKeyDeserializer
+  it should "round-trip Int.MaxValue but reject an out-of-Int-range portId on deserialize" in {
+    // Upper boundary of the portId domain round-trips intact.
+    val p = globalPort(portIdValue = Int.MaxValue)
+    GlobalPortIdentitySerde.deserializeFromString(p.serializeAsString) shouldBe p
+    // A value past Int range fails in `.toInt` with NumberFormatException.
+    val overflow =
+      "(logicalOpId=op-A,layerName=main,portId=9999999999,isInternal=false,isInput=true)"
     intercept[NumberFormatException] {
-      d.deserializeKey("missingSeparator", null)
+      GlobalPortIdentitySerde.deserializeFromString(overflow)
     }
   }
 
-  it should "throw ArrayIndexOutOfBoundsException when only the id is provided (no `_internal` suffix)" in {
-    // Different separator-missing path: `\"5\".split(\"_\")` yields
-    // [\"5\"], parts(0).toInt = 5 succeeds, then parts(1) reads past the
-    // end. Pin this failure mode explicitly so a future safer parser
-    // breaks the spec on purpose (and the safer error type is chosen
-    // consciously).
-    val d = new PortIdentityKeyDeserializer
-    intercept[ArrayIndexOutOfBoundsException] {
-      d.deserializeKey("5", null)
-    }
+  it should "accept mixed-case boolean fields on deserialize (String.toBoolean is case-insensitive)" in {
+    // `String.toBoolean` accepts any case variant of true/false, so a
+    // tampered/legacy string with `True` / `FALSE` still parses.
+    val s = "(logicalOpId=op-A,layerName=main,portId=0,isInternal=True,isInput=FALSE)"
+    val restored = GlobalPortIdentitySerde.deserializeFromString(s)
+    restored.portId.internal shouldBe true
+    restored.input shouldBe false
   }
 
-  it should "silently accept extra trailing underscore-separated segments (lenient parser, current behavior)" in {
-    // Pin the current lenient behavior: `parts(0).toInt` and
-    // `parts(1).toBoolean` ignore everything past `parts(1)`, so a key
-    // like `"1_true_garbage"` deserializes to `PortIdentity(1, true)`
-    // without complaint. The strict-rejection variant lives in a
-    // pendingUntilFixed test below; characterizing today's lenient
-    // path here means a future-tightening fix would need to update
-    // both tests deliberately.
-    val d = new PortIdentityKeyDeserializer
-    val pid = d.deserializeKey("1_true_garbage", null)
-    assert(pid == PortIdentity(1, internal = true))
+  it should "round-trip a value containing '=' (only ',' is a sensitive separator)" in {
+    // Counterpart to the comma case above: the deserializer anchors on the
+    // literal `logicalOpId=` prefix and then captures up to the next comma
+    // (`[^,]+`), so an embedded `=` is harmless and round-trips intact.
+    // Pin this so a future regex change that special-cased `=` would fail.
+    val p = globalPort(logical = "a=b")
+    GlobalPortIdentitySerde.deserializeFromString(p.serializeAsString) shouldBe p
   }
 
-  it should "eventually reject keys with extra trailing segments (pendingUntilFixed)" in pendingUntilFixed {
-    // Documented contract: a `PortIdentityKeySerializer` output is exactly
-    // `id_internal` — two underscore-separated segments. Anything else is
-    // corrupt JSON and should be rejected, not silently truncated. The
-    // current implementation is lenient (see characterization test
-    // above); this pendingUntilFixed flips to passing once the parser
-    // is hardened, then `pendingUntilFixed` inverts that into a
-    // deliberate failure forcing the marker to be removed.
-    val d = new PortIdentityKeyDeserializer
+  it should "serialize an empty logicalOpId but fail to deserialize the result (serialize does not guard emptiness)" in {
+    // The serializer only rejects underscores and negative portIds, not empty
+    // identifiers, so an empty logicalOpId serializes into `(logicalOpId=,...)`.
+    // The deserializer's `[^,]+` requires at least one character, so that
+    // output can no longer be parsed back — the serialize side of the same
+    // asymmetry the "empty field body" deserialize test characterizes.
+    val s = globalPort(logical = "").serializeAsString
     intercept[IllegalArgumentException] {
-      d.deserializeKey("1_true_garbage", null)
+      GlobalPortIdentitySerde.deserializeFromString(s)
     }
   }
+
+  it should "name the offending field in the require failure message" in {
+    // The three serialize-side guards throw IllegalArgumentException; assert
+    // the message identifies which field failed so a regression that wires a
+    // check to the wrong field (e.g. validating logicalOpId in layerName's
+    // guard) is caught instead of silently still throwing.
+    intercept[IllegalArgumentException] {
+      globalPort(logical = "__x").serializeAsString
+    }.getMessage should include("logicalOpId")
+    intercept[IllegalArgumentException] {
+      globalPort(layer = "a_b").serializeAsString
+    }.getMessage should include("layerName")
+    intercept[IllegalArgumentException] {
+      globalPort(portIdValue = -1).serializeAsString
+    }.getMessage should include("portId")
+  }
 }
diff --git a/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentitySerdeSpec.scala b/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentitySerdeSpec.scala
index 89a815c39e..e7f6ad037b 100644
--- a/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentitySerdeSpec.scala
+++ b/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentitySerdeSpec.scala
@@ -19,200 +19,12 @@
 
 package org.apache.texera.amber.util.serde
 
-import org.apache.texera.amber.core.virtualidentity.{OperatorIdentity, PhysicalOpIdentity}
-import org.apache.texera.amber.core.workflow.{GlobalPortIdentity, PortIdentity}
+import org.apache.texera.amber.core.workflow.PortIdentity
 import org.apache.texera.amber.util.JSONUtils.objectMapper
-import org.apache.texera.amber.util.serde.GlobalPortIdentitySerde.SerdeOps
 import org.scalatest.flatspec.AnyFlatSpec
 
 class PortIdentitySerdeSpec extends AnyFlatSpec {
 
-  // ---------------------------------------------------------------------------
-  // GlobalPortIdentitySerde
-  // ---------------------------------------------------------------------------
-
-  private def globalPort(
-      logical: String = "op-A",
-      layer: String = "main",
-      portIdValue: Int = 0,
-      internal: Boolean = false,
-      input: Boolean = true
-  ): GlobalPortIdentity =
-    GlobalPortIdentity(
-      opId = PhysicalOpIdentity(OperatorIdentity(logical), layer),
-      portId = PortIdentity(id = portIdValue, internal = internal),
-      input = input
-    )
-
-  "GlobalPortIdentitySerde" should "round-trip a default GlobalPortIdentity through serializeAsString → deserializeFromString" in {
-    val original = globalPort()
-    val restored = GlobalPortIdentitySerde.deserializeFromString(original.serializeAsString)
-    assert(restored == original)
-  }
-
-  it should "preserve all five fields independently across the round-trip" in {
-    // Vary each field individually so a regression that swapped two fields
-    // (e.g., isInput / isInternal) would surface here, not as a general
-    // round-trip failure.
-    val cases = Seq(
-      globalPort(logical = "op-A"),
-      globalPort(logical = "op-Z"),
-      globalPort(layer = "main"),
-      globalPort(layer = "extra-layer"),
-      globalPort(portIdValue = 0),
-      globalPort(portIdValue = 7),
-      globalPort(internal = false),
-      globalPort(internal = true),
-      globalPort(input = true),
-      globalPort(input = false)
-    )
-    cases.foreach { p =>
-      val s = p.serializeAsString
-      val restored = GlobalPortIdentitySerde.deserializeFromString(s)
-      assert(restored == p, s"round-trip mismatch for $p (serialized: $s)")
-    }
-  }
-
-  it should "produce the documented format for default and non-default values" in {
-    // Pin the exact format. If this changes, callers reading existing
-    // VFS URIs from disk will break — locking it down forces a deliberate
-    // migration story.
-    assert(
-      globalPort().serializeAsString ==
-        "(logicalOpId=op-A,layerName=main,portId=0,isInternal=false,isInput=true)"
-    )
-    assert(
-      globalPort(
-        logical = "op-Z",
-        layer = "extra-layer",
-        portIdValue = 7,
-        internal = true,
-        input = false
-      ).serializeAsString ==
-        "(logicalOpId=op-Z,layerName=extra-layer,portId=7,isInternal=true,isInput=false)"
-    )
-  }
-
-  it should "round-trip identifiers containing dashes and dots (regex non-comma matcher)" in {
-    // The deserialization regex uses `[^,]+` for the field body, so any
-    // non-comma character is fair game. Cover the realistic counter-
-    // examples (dashes, dots) since logical op ids and layer names use
-    // both; if the regex were ever tightened to alphanumerics only, this
-    // would fail on purpose.
-    val p = globalPort(logical = "my.op-with-dashes.v2", layer = "main-1")
-    assert(GlobalPortIdentitySerde.deserializeFromString(p.serializeAsString) == p)
-  }
-
-  it should "throw IllegalArgumentException when serializing a negative port id" in {
-    // Port ids are array indices and must be non-negative; the serializer
-    // rejects negatives so corrupt data can't reach VFS URIs.
-    intercept[IllegalArgumentException] {
-      globalPort(portIdValue = -1).serializeAsString
-    }
-  }
-
-  it should "throw IllegalArgumentException when deserializing a negative port id" in {
-    // Symmetric: a hand-crafted string with a negative portId must be
-    // rejected by the deserializer too (so tampered URIs don't slip
-    // through).
-    val malformed = "(logicalOpId=op-A,layerName=main,portId=-1,isInternal=false,isInput=true)"
-    intercept[IllegalArgumentException] {
-      GlobalPortIdentitySerde.deserializeFromString(malformed)
-    }
-  }
-
-  it should "throw IllegalArgumentException when the input has the wrong field order" in {
-    // The regex pins the documented field order; a swapped order should
-    // not silently parse with confused values.
-    val swapped = "(layerName=main,logicalOpId=op-A,portId=0,isInternal=false,isInput=true)"
-    intercept[IllegalArgumentException] {
-      GlobalPortIdentitySerde.deserializeFromString(swapped)
-    }
-  }
-
-  it should "throw IllegalArgumentException when the input has trailing content past the closing paren" in {
-    val withTrailing =
-      "(logicalOpId=op-A,layerName=main,portId=0,isInternal=false,isInput=true) extra"
-    intercept[IllegalArgumentException] {
-      GlobalPortIdentitySerde.deserializeFromString(withTrailing)
-    }
-  }
-
-  it should "throw IllegalArgumentException when a field body is empty" in {
-    // `[^,]+` requires at least one character, so an empty layerName
-    // (`,layerName=,`) must fail to match.
-    val emptyLayer = "(logicalOpId=op-A,layerName=,portId=0,isInternal=false,isInput=true)"
-    intercept[IllegalArgumentException] {
-      GlobalPortIdentitySerde.deserializeFromString(emptyLayer)
-    }
-  }
-
-  it should "throw IllegalArgumentException on a completely malformed string" in {
-    val ex = intercept[IllegalArgumentException] {
-      GlobalPortIdentitySerde.deserializeFromString("not even close")
-    }
-    assert(ex.getMessage.contains("not even close"))
-  }
-
-  it should "throw IllegalArgumentException when a required field is missing" in {
-    // Drop isInput.
-    val malformed = "(logicalOpId=op-A,layerName=main,portId=0,isInternal=false)"
-    intercept[IllegalArgumentException] {
-      GlobalPortIdentitySerde.deserializeFromString(malformed)
-    }
-  }
-
-  it should "throw NumberFormatException when portId is non-numeric" in {
-    // The regex matches (`[^,]+`) but `.toInt` fails. NumberFormatException
-    // extends IllegalArgumentException; assert the more specific type so a
-    // regression that swallowed/rewrapped it is visible.
-    val malformed = "(logicalOpId=op-A,layerName=main,portId=NaN,isInternal=false,isInput=true)"
-    intercept[NumberFormatException] {
-      GlobalPortIdentitySerde.deserializeFromString(malformed)
-    }
-  }
-
-  it should "throw IllegalArgumentException when a boolean field is non-boolean" in {
-    // `String.toBoolean` is strict: only \"true\" / \"false\" (case-insensitive)
-    // pass; anything else throws IllegalArgumentException.
-    val malformed = "(logicalOpId=op-A,layerName=main,portId=0,isInternal=maybe,isInput=true)"
-    intercept[IllegalArgumentException] {
-      GlobalPortIdentitySerde.deserializeFromString(malformed)
-    }
-  }
-
-  it should "use no underscore in its own format characters (separators / keys)" in {
-    // Pin the format-character invariant: the wrapping `(...)`, the field
-    // separators `,`, the key=value separators, and the field NAMES
-    // themselves contain no underscore. Verify by building the format with
-    // empty-string-replacement values for every input field, so anything
-    // left in the output is purely from `serializeAsString`'s own format.
-    // (For the layerName field the empty-input variant is rejected by the
-    // deserializer regex; here we only check the SERIALIZED output, not the
-    // round-trip.)
-    val s = globalPort(logical = "x", layer = "x").serializeAsString
-    val formatChars = s.replace("x", "").replace("0", "").replace("false", "").replace("true", "")
-    assert(!formatChars.contains("_"), s"format characters must be underscore-free: $formatChars")
-  }
-
-  it should "throw IllegalArgumentException when logicalOpId contains an underscore" in {
-    // Enforces the documented VFS-compatibility contract: the serialized
-    // form must be underscore-free. The serializer rejects underscored
-    // inputs at the boundary instead of silently emitting a string that
-    // would interfere with VFS URI parsing downstream.
-    intercept[IllegalArgumentException] {
-      globalPort(logical = "__DummyOperator").serializeAsString
-    }
-  }
-
-  it should "throw IllegalArgumentException when layerName contains an underscore" in {
-    // Both fields enforce the same invariant; cover them independently so
-    // a partial fix that only validates one surfaces as a test failure.
-    intercept[IllegalArgumentException] {
-      globalPort(layer = "main_source_0_op").serializeAsString
-    }
-  }
-
   // ---------------------------------------------------------------------------
   // PortIdentityKeySerializer.portIdToString (companion, not the Jackson class)
   // ---------------------------------------------------------------------------