From a297c1acad27ed3de66b8bed51ac977958d6425a Mon Sep 17 00:00:00 2001 From: Xinyuan Lin Date: Sun, 7 Jun 2026 17:00:29 -0700 Subject: [PATCH] test(amber): add unit test coverage for FutureBijection and ElidableStatement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pins behavior of two utility modules in `engine/common`: - `FutureBijection` — implicit Twitter↔Scala Future conversions; pin the value and exception paths in both directions, plus round-trip preservation - `ElidableStatement` — pin the silent-in-production contract that every helper (FINEST/FINER/FINE/INFO) is elided under the build's `-Xelide-below WARNING` flag, so by-name blocks never execute Closes #5551 --- .../engine/common/ElidableStatementSpec.scala | 141 ++++++++++++++++++ .../engine/common/FutureBijectionSpec.scala | 139 +++++++++++++++++ 2 files changed, 280 insertions(+) create mode 100644 amber/src/test/scala/org/apache/texera/amber/engine/common/ElidableStatementSpec.scala create mode 100644 amber/src/test/scala/org/apache/texera/amber/engine/common/FutureBijectionSpec.scala 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 00000000000..6f7e2eb2b33 --- /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 00000000000..1ff59594dd3 --- /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) + } +}