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-5555-397d2757f3094818c96681261324cc9a9ff17763 in repository https://gitbox.apache.org/repos/asf/texera.git
commit d76a51e347f54c6c3ff43a7f8cd11f14ae5739ea Author: Xinyuan Lin <[email protected]> AuthorDate: Fri Jun 12 10:06:53 2026 -0700 test(amber): add unit test coverage for FutureBijection and ElidableStatement (#5555) ### What changes were proposed in this PR? Pin behavior of two utility modules in `engine/common`. No production-code changes. | Spec | Source class | Tests | | --- | --- | --- | | `FutureBijectionSpec` | `FutureBijection` | 11 | | `ElidableStatementSpec` | `ElidableStatement` | 9 | Both spec files follow the `<srcClassName>Spec.scala` one-to-one convention. **Behavior pinned — `FutureBijection`** | Surface | Contract | | --- | --- | | `TwitterFuture.value.asScala` | resolves to the same value (type preserved, `null` preserved) | | `TwitterFuture.exception.asScala` | resolves with the same `Throwable` instance (type, message, `eq` identity) | | `ScalaFuture.successful.asTwitter` | resolves to the same value (type preserved, `null` preserved) | | `ScalaFuture.failed.asTwitter` | resolves with the same `Throwable` instance | | Twitter → Scala on an already-resolved future | the resulting Scala future is already completed when the implicit returns | | Twitter → Scala → Twitter round-trip | preserves both values and exceptions | | Scala → Twitter → Scala round-trip | preserves values | **Behavior pinned — `ElidableStatement`** The texera build sets `-Xelide-below WARNING` (`amber/build.sbt`). Every `ElidableStatement` helper is annotated with an elide level **strictly below WARNING** (FINEST / FINER / FINE / INFO), so the Scala compiler replaces every CALL to these helpers with a `()` Unit value at *compile* time. The spec pins this silent-in-production contract: | Surface | Contract | | --- | --- | | `info` / `fine` / `finer` / `finest` (with side-effect block) | the side effect MUST NOT fire (counter stays at 0) | | same methods (with throwing block) | the exception MUST NOT propagate | | 1000 successive elided calls | no side-effect accumulation | | Return type | `Unit` (compile-time enforced) | | Parameter shape | accepts a `=> Unit` by-name block (compile-time enforced) | A regression that bumped a method's elide level above WARNING, removed the `@elidable` annotation, or relaxed `-Xelide-below` in the build would re-enable side effects in production — and this spec would catch it. ### Any related issues, documentation, discussions? Closes #5551. ### How was this PR tested? Pure unit-test additions; verified locally with: - `sbt "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.common.FutureBijectionSpec org.apache.texera.amber.engine.common.ElidableStatementSpec"` — 20 tests, all green - `sbt scalafmtCheckAll` — clean - CI to confirm ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Sonnet 4.5) --- .../engine/common/ElidableStatementSpec.scala | 141 +++++++++++++++++++++ .../amber/engine/common/FutureBijectionSpec.scala | 139 ++++++++++++++++++++ 2 files changed, 280 insertions(+) diff --git a/amber/src/test/scala/org/apache/texera/amber/engine/common/ElidableStatementSpec.scala b/amber/src/test/scala/org/apache/texera/amber/engine/common/ElidableStatementSpec.scala new file mode 100644 index 0000000000..6f7e2eb2b3 --- /dev/null +++ b/amber/src/test/scala/org/apache/texera/amber/engine/common/ElidableStatementSpec.scala @@ -0,0 +1,141 @@ +/* + * 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.engine.common + +import org.scalatest.flatspec.AnyFlatSpec + +class ElidableStatementSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Context — the texera build sets `-Xelide-below WARNING` (see + // `amber/build.sbt`). Every `ElidableStatement` helper is annotated with + // an elide level strictly below WARNING (FINEST / FINER / FINE / INFO), + // so the Scala compiler replaces every CALL to these helpers with a + // `()` Unit value at *compile* time. The by-name block argument is + // never even constructed, let alone evaluated, in production / test + // builds — that is the entire point of the abstraction. + // + // This spec pins that contract: a regression that bumped a method's + // elide level above WARNING (e.g. `@elidable(SEVERE)`), removed the + // `@elidable` annotation, or relaxed `-Xelide-below` in the build + // would re-enable side effects and break the silent-in-production + // promise — and this spec would catch it. + // --------------------------------------------------------------------------- + + // --------------------------------------------------------------------------- + // Each helper compiles to a no-op (block side effect does NOT fire) + // --------------------------------------------------------------------------- + + "ElidableStatement.finest" should + "be elided at the build's elide level — its by-name block must NOT execute" in { + var counter = 0 + ElidableStatement.finest { counter += 1 } + assert(counter == 0, "block should be elided away, counter must remain at 0") + } + + "ElidableStatement.finer" should + "be elided at the build's elide level — its by-name block must NOT execute" in { + var counter = 0 + ElidableStatement.finer { counter += 1 } + assert(counter == 0) + } + + "ElidableStatement.fine" should + "be elided at the build's elide level — its by-name block must NOT execute" in { + var counter = 0 + ElidableStatement.fine { counter += 1 } + assert(counter == 0) + } + + "ElidableStatement.info" should + "be elided at the build's elide level — its by-name block must NOT execute" in { + var counter = 0 + ElidableStatement.info { counter += 1 } + assert(counter == 0) + } + + // --------------------------------------------------------------------------- + // Even a throwing block must NOT propagate — it's never evaluated. + // --------------------------------------------------------------------------- + + "Elided helpers" should + "not propagate an exception that would have been thrown by their block" in { + // If `info` accidentally stopped being elided, this would re-raise the + // RuntimeException and fail the test. Pinning the suppression directly + // catches that regression. + ElidableStatement.info { throw new RuntimeException("must never fire") } + ElidableStatement.fine { throw new RuntimeException("must never fire") } + ElidableStatement.finer { throw new RuntimeException("must never fire") } + ElidableStatement.finest { throw new RuntimeException("must never fire") } + succeed + } + + // --------------------------------------------------------------------------- + // Multiple calls don't accumulate side effects (each one is independently + // elided). + // --------------------------------------------------------------------------- + + "Repeated elided calls" should "stay no-ops across 1000 invocations" in { + var counter = 0 + var i = 0 + while (i < 1000) { + ElidableStatement.info { counter += 1 } + i += 1 + } + assert( + counter == 0, + s"1000 elided info calls should not accumulate side effects, got: $counter" + ) + } + + // --------------------------------------------------------------------------- + // Return-type contract — each helper still type-checks as `=> Unit ⇒ Unit`. + // --------------------------------------------------------------------------- + + "ElidableStatement methods" should "all return Unit (compile-time enforced)" in { + // Assignments would fail to typecheck if a method's signature drifted + // — e.g. someone made `info` return the block's result. The fact that + // these compile under `-Xelide-below WARNING` also confirms each call + // is replaced with the Unit `()` value, not with an exception. + val r1: Unit = ElidableStatement.info { () } + val r2: Unit = ElidableStatement.fine { () } + val r3: Unit = ElidableStatement.finer { () } + val r4: Unit = ElidableStatement.finest { () } + assert(r1 == r2 && r2 == r3 && r3 == r4) + } + + // --------------------------------------------------------------------------- + // By-name parameter shape — each helper accepts a `=> Unit` block + // (verified at compile time by passing a parameter-less lambda body). + // --------------------------------------------------------------------------- + + "ElidableStatement methods" should "accept a by-name `=> Unit` argument (compile-time enforced)" in { + // The fact that these expressions compile proves the parameter shape: + // a value-typed expression of type Unit AND a thunk that runs side + // effects are both accepted. Under `-Xelide-below WARNING`, neither + // executes — but the type contract still holds. + ElidableStatement.info { () } + ElidableStatement.info { val x = 1; val y = x + 1; () } + ElidableStatement.info { + println("debug") + } + succeed + } +} diff --git a/amber/src/test/scala/org/apache/texera/amber/engine/common/FutureBijectionSpec.scala b/amber/src/test/scala/org/apache/texera/amber/engine/common/FutureBijectionSpec.scala new file mode 100644 index 0000000000..1ff59594dd --- /dev/null +++ b/amber/src/test/scala/org/apache/texera/amber/engine/common/FutureBijectionSpec.scala @@ -0,0 +1,139 @@ +/* + * 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.engine.common + +import com.twitter.util.{Await => TwitterAwait, Future => TwitterFuture} +import org.apache.texera.amber.engine.common.FutureBijection._ +import org.scalatest.flatspec.AnyFlatSpec + +import scala.concurrent.duration._ +import scala.concurrent.{Await => ScalaAwait, Future => ScalaFuture} + +class FutureBijectionSpec extends AnyFlatSpec { + + // Short timeout — the futures complete synchronously (or near-so) for + // the values used here; we keep the bound tight so a regression that + // *fails to resolve* surfaces as a timeout rather than hanging CI. + private val timeout: FiniteDuration = 5.seconds + private val twitterTimeout: com.twitter.util.Duration = com.twitter.util.Duration.fromSeconds(5) + + // --------------------------------------------------------------------------- + // Twitter Future → Scala Future + // --------------------------------------------------------------------------- + + "RichTwitterFuture.asScala" should "resolve to the same value as the wrapped TwitterFuture" in { + val tf = TwitterFuture.value(42) + val result = ScalaAwait.result(tf.asScala, timeout) + assert(result == 42) + } + + it should "preserve the exception type and message on the failure path" in { + val cause = new IllegalStateException("boom") + val tf = TwitterFuture.exception[Int](cause) + val ex = intercept[IllegalStateException] { + ScalaAwait.result(tf.asScala, timeout) + } + assert(ex.getMessage == "boom") + assert(ex eq cause, "the same Throwable instance should propagate through") + } + + it should "preserve a null value on the value path (Twitter Return wraps any AnyRef)" in { + // The conversion calls `promise.success(value)` directly; for a `null` + // value, the Scala promise resolves to `null`. Pin that the wire does + // not coerce null into an exception. + val tf = TwitterFuture.value[String](null) + val result = ScalaAwait.result(tf.asScala, timeout) + assert(result == null) + } + + it should "preserve the value type (compile-time enforced)" in { + val tf: TwitterFuture[String] = TwitterFuture.value("hello") + val sf: ScalaFuture[String] = tf.asScala + assert(ScalaAwait.result(sf, timeout) == "hello") + } + + it should "produce a future that has already completed when the source TwitterFuture is already resolved" in { + val tf = TwitterFuture.value(7) + val sf = tf.asScala + // The conversion uses `respond` which fires synchronously for already- + // resolved futures, so the scala future is complete by the time the + // implicit returns. + assert(sf.isCompleted, "the converted future should be completed immediately") + } + + // --------------------------------------------------------------------------- + // Scala Future → Twitter Future + // --------------------------------------------------------------------------- + + "RichScalaFuture.asTwitter" should "resolve to the same value as the wrapped ScalaFuture" in { + val sf = ScalaFuture.successful(42) + val tf = sf.asTwitter() + assert(TwitterAwait.result(tf, twitterTimeout) == 42) + } + + it should "preserve the exception type and message on the failure path" in { + val cause = new IllegalArgumentException("nope") + val sf = ScalaFuture.failed[Int](cause) + val ex = intercept[IllegalArgumentException] { + TwitterAwait.result(sf.asTwitter(), twitterTimeout) + } + assert(ex.getMessage == "nope") + assert(ex eq cause, "the same Throwable instance should propagate through") + } + + it should "preserve a null value on the value path" in { + val sf = ScalaFuture.successful[String](null) + val tf = sf.asTwitter() + assert(TwitterAwait.result(tf, twitterTimeout) == null) + } + + it should "preserve the value type (compile-time enforced)" in { + val sf: ScalaFuture[String] = ScalaFuture.successful("hello") + val tf: TwitterFuture[String] = sf.asTwitter() + assert(TwitterAwait.result(tf, twitterTimeout) == "hello") + } + + // --------------------------------------------------------------------------- + // Round-trip — Twitter → Scala → Twitter and vice versa + // --------------------------------------------------------------------------- + + "FutureBijection" should "round-trip a value through Twitter → Scala → Twitter" in { + val tf1 = TwitterFuture.value("payload") + val tf2: TwitterFuture[String] = tf1.asScala.asTwitter() + assert(TwitterAwait.result(tf2, twitterTimeout) == "payload") + } + + it should "round-trip a value through Scala → Twitter → Scala" in { + val sf1 = ScalaFuture.successful("payload") + val sf2: ScalaFuture[String] = sf1.asTwitter().asScala + assert(ScalaAwait.result(sf2, timeout) == "payload") + } + + it should "round-trip an exception through Twitter → Scala → Twitter (type + message preserved)" in { + val cause = new RuntimeException("round-trip-err") + val tf1 = TwitterFuture.exception[Int](cause) + val tf2: TwitterFuture[Int] = tf1.asScala.asTwitter() + val ex = intercept[RuntimeException] { + TwitterAwait.result(tf2, twitterTimeout) + } + assert(ex.getMessage == "round-trip-err") + assert(ex eq cause) + } +}
