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/09/09 06:04:50 UTC
(texera) 01/01: test(pybuilder): make the bad-neighbour boundary sweep actually compile its snippets (#8405)
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-8405-bda3400261d38e674086f3b44cfcae169ccc55e5
in repository https://gitbox.apache.org/repos/asf/texera.git
commit ec3a9dd3ca9c361cbd7f339d8bc78669024e84e8
Author: Xinyuan Lin <xi...@uci.edu>
AuthorDate: Wed Sep 9 06:04:25 2026 +0000
test(pybuilder): make the bad-neighbour boundary sweep actually compile its snippets (#8405)
### What changes were proposed in this PR?
`PythonTemplateBuilderSpec`'s exhaustive bad-neighbour test passed
vacuously. Its private helper wrapped every snippet in a package clause
before handing it to a runtime `ToolBox`:
```scala
private def inPybuilderPkg(code: String): String =
s"""package org.apache.texera.amber.pybuilder {
|$code
|}""".stripMargin
private def assertToolboxDoesNotCompile(code: String): Unit = {
intercept[Throwable] { tb.compile(tb.parse(inPybuilderPkg(code))) }
()
}
```
A `ToolBox` cannot wrap a `PackageDef` into its synthetic `__wrapper`
method, so `tb.compile` dies during typer with a bare
`java.lang.AssertionError` for **any** input. Note the type: not
`ToolBoxError`, which is why `intercept[Throwable]` was needed for the
helper to pass.
I ran the helper against inputs that obviously must not be rejected.
Every one "does not compile":
| input fed to the helper | `tb.parse` | `tb.compile` |
|---|---|---|
| `""` (the empty string) | OK -> `PackageDef` |
`java.lang.AssertionError: assertion failed: method wrapper` |
| `object Trivial { val x = 1 }` | OK -> `PackageDef` | same |
| a correct `pyb"foo $ui bar"` (whitespace neighbours) | OK ->
`PackageDef` | same |
| `,` as a left neighbour — a *good* neighbour | OK -> `PackageDef` |
same |
| syntactically broken garbage | throws `ToolBoxError` | (not reached) |
The measured message body is `assertion failed: / method wrapper / while
compiling: <no file> / during phase: typer / library version: version
2.13.18 / compiler version: version 2.13.18 / reconstructed args: / last
tree to typer: ...`. The compilation dies wrapping the tree, before
macro expansion. `tb.parse` on the same input succeeds, so the failure
is entirely in `tb.compile`. Compiling the identical body as a *block*
instead of a package clause reaches the macro and aborts properly, which
is what the rewrite relies on.
The helper was used at two call sites, inside the test that iterates the
`isBadNeighbor` subset of printable ASCII and asserts left- and
right-adjacency per character. All 130 of those assertions passed
without the `pyb` macro ever expanding.
**Before -> after:**
```
before: assertToolboxDoesNotCompile(<package block>) -> AssertionError, always, for every input
after: macroError(<block>) -> ToolBoxError whose MESSAGE says which rule fired
```
The rewrite uses the technique `BoundaryValidatorSpec` already documents
in its header: compile a *block*, not a package; the snippet always
fails (the expansion calls the `private[amber]` `fromInterpolated`,
which the ToolBox's `__wrapper` package cannot reach), but the macro
fully expands first, so the two outcomes are distinguishable by message
— a `validateCompileTime` abort carrying the specific boundary reason,
versus a benign expansion whose only failure is the `fromInterpolated`
access error.
Concretely:
- Each of the 65 bad-neighbour characters is asserted twice — once as
the left neighbour, once as the right — to abort with the abort marker
*and* its own templated reason, e.g. ``must not be immediately adjacent
to 'z' on the left``. 130 real macro expansions.
- New test, the discriminating direction: the 29 safe-neighbour
characters must produce the *benign* outcome (57 cases; `#` on the left
is excluded and pinned separately). Without this, weakening the
neighbour rule to "always abort" would leave the first sweep green.
- New test: `#` as a left neighbour aborts for the comment rule, not the
neighbour rule.
- The character sets are spelled out rather than derived from
`PythonLexerUtils.isBadNeighbor`. Deriving them was a second, subtler
vacuity: shrinking the predicate would silently shrink the sweep's input
instead of failing it.
- Removed: `inPybuilderPkg`, `assertToolboxDoesNotCompile`, and
`scalaUnicodeEscape`, which had no other user. The escape helper was
broken on its own terms too: it emitted `\\u0041` — **two** backslashes
— into the generated source, nothing rewrites that inside the generated
triple-quoted literal, and the abort message proves what the macro
actually saw: the left neighbour was the digit `1`, never `A`. So the
old sweep fed the same wrong neighbour for all 65 characters. The new
snippets embed the raw character instead.
**What this PR does not do:** it touches no production code and adds no
tests to any other area. Measured from the JUnit XML, the file goes from
65 to 67 test cases: 1 test rewritten, 2 added, 0 removed and 0 renamed,
so **64** tests, their names and the file's structure are untouched. The
file contains **nine** `assertDoesNotCompile` tests using ScalaTest's
own macro (not the broken helper), **four** of them single-case
adjacency tests; those nine are unchanged, and three of the four
adjacency ones demonstrably fire under Mutation C below.
### Any related issues, documentation, discussions?
Closes #8401
### How was this PR tested?
Baseline on `1cbe857007`, `PyBuilder/test`: **184 tests, 5 suites, 0
failures**. After: **186 tests, 5 suites, 0 failures**. Comparing
test-case identities from
`common/pybuilder/target/test-reports/TEST-*.xml` rather than counts: 0
removed, 0 renamed, and the 2 additions are exactly the new tests named
above. Every suite reports `failures="0"` on both sides.
`PyBuilder/scalafmtCheck`, `PyBuilder/Test/scalafmtCheck` and
`PyBuilder/scalafixAll --check` all pass.
Non-vacuity, both directions, measured:
| check | result |
|---|---|
| bad-neighbour sweep | 130 of 130 cases abort with the expected reason
*and* the expected character |
| safe-neighbour sweep | 57 of 57 cases benign (no abort marker,
`fromInterpolated` present) |
| `#` left neighbour | aborts with the comment reason, not a neighbour
reason |
| real ToolBox compilations | 187 (130 + 57), each one a genuine macro
expansion |
Both sweeps carry loop-ran guards (`assert(checked == 130)` /
`assert(checked == 57)`) and set-size guards (`assert(size == 65)` /
`assert(size == 29)`), and report every mismatching case rather than
failing fast, so a regression names the characters.
Mutation A — `PythonLexerUtils.isBadNeighbor` changed to `... ||
(isIdentChar(c) && c != 'z')`:
```
rewritten test: RED - 1 test failed, "2 of 130 adjacency cases did not abort with the neighbour reason":
left [z] (U+007A): ... method fromInterpolated ... cannot be accessed ...
right [z] (U+007A): ... method fromInterpolated ... cannot be accessed ...
old test, same mutation: GREEN - PyBuilder/test = 184 succeeded, 0 failed, 5 suites
```
That contrast is the clearest evidence here: under a mutation that
removes one character from the bad-neighbour set, the old test and every
other suite in the module stayed green; the rewritten sweep names the
exact character, both sides, and quotes the benign message as the
reason. `PythonLexerUtilsSpec` does not catch it either — it only
*samples* the predicate (`'`, `"`, `a`, `Z`, `0`, `_`, plus two
negatives) and never tests `z`. The rewritten sweep catches it only
because the character set is no longer derived from the predicate.
Mutation B — the right-neighbour abort in
`BoundaryValidator.validateCompileTime` disabled (`if (false &&
isBadNeighbor(rightNeighbor))`):
```
rewritten test: RED - "65 of 130 adjacency cases" (exactly the right-side arm; the 65 left-side cases still pass)
```
Mutation C — *both* compile-time neighbour arms disabled:
```
rewritten test: RED - "130 of 130 adjacency cases"
also RED: 3 pre-existing single-case tests -
"UI glued to identifier on the left does not compile"
"UI glued to identifier on the right does not compile"
"UI glued to a quote on the right does not compile"
total: 4 failed, 63 succeeded
```
The fourth adjacency test ("PyString (EncodableString) glued to
identifier on the left") stays green under Mutation C — it is pinned
through a different path, not the compile-time neighbour arms.
All three mutations were reverted by copying back pre-mutation file
copies, never `git checkout`/`git restore`. `git diff 1cbe857007 --
'*/src/main/*'` is empty on the committed branch.
**Corrections after review.** Three claims in an earlier draft of this
description were wrong and are fixed above; recording them rather than
editing them away:
| earlier claim | measured |
|---|---|
| "the other 63 tests ... untouched" | 64 (65 -> 67 cases, 1 rewritten,
2 added, 0 removed) |
| "Two pre-existing single-case boundary tests" | 9
`assertDoesNotCompile` tests, 4 of them adjacency; "two" was the count
that happened to fire under Mutation B, not a property of the file |
| the escape helper "fed `A` ... as six literal characters" | it emitted
`\\u0041`, two backslashes; the macro's neighbour was the digit `1` |
A fourth: an earlier draft put the sweep runtime at "~4.2 s". Wall-clock
for ToolBox work is not stable enough to quote — three runs on the same
machine measured the two sweeps at 4.02 s, 4.90 s and 5.70 s — so the
figure is dropped in favour of the compilation count, which is exact.
One thing worth recording, because it looks like a bug and is not:
`pyb"pre ${ui}\" post"` — a `"` written as a Scala escape — is *not*
rejected. `StringContext.parts` for a custom interpolator are raw, so
both the validator and `fromInterpolated` see `\` as the neighbour, and
`fromInterpolated` is documented as taking raw parts and does not call
`processEscapes`. The validator and the renderer agree, so there is
nothing to fix. The new snippets sidestep it by using triple-quoted
Scala literals, where the character is verbatim.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
---------
Signed-off-by: Xinyuan Lin <xi...@uci.edu>
Co-authored-by: Copilot Autofix powered by AI <17...@users.noreply.github.com>
---
.../pybuilder/PythonTemplateBuilderSpec.scala | 215 ++++++++++++++++-----
1 file changed, 165 insertions(+), 50 deletions(-)
diff --git a/common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala b/common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala
index 727f5cf14c..71810f9316 100644
--- a/common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala
+++ b/common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala
@@ -31,7 +31,7 @@ import java.nio.charset.StandardCharsets
import java.util.Base64
import scala.annotation.meta.field
import scala.reflect.runtime.currentMirror
-import scala.tools.reflect.ToolBox
+import scala.tools.reflect.{ToolBox, ToolBoxError}
class PythonTemplateBuilderSpec extends AnyFunSuite {
@@ -41,26 +41,100 @@ class PythonTemplateBuilderSpec extends AnyFunSuite {
private def decodeExpr(text: String): String =
PythonTemplateBuilder.wrapWithPythonDecoderExpr(base64Of(text))
- // Toolbox helpers: used to assert runtime exceptions without checking error strings.
+ // ------------------------------------------------------------------------
+ // ToolBox harness for the exhaustive boundary sweeps further down.
+ //
+ // A `pyb` snippet can only be *compiled* (never `eval`-run) through a ToolBox: the expansion
+ // calls the `private[amber]` `fromInterpolated`, which the ToolBox's synthetic `__wrapper`
+ // package cannot access. Compilation is enough, because the macro fully expands (running
+ // `BoundaryValidator.validateCompileTime`) before that access error surfaces. So every snippet
+ // fails, and the two outcomes are told apart by the captured `ToolBoxError` *message*:
+ //
+ // - an abort, whose message carries the specific boundary reason, versus
+ // - a benign expansion whose only failure is the `fromInterpolated` access error.
+ //
+ // The snippet must be a *block*, never a `package` clause: the ToolBox cannot wrap a PackageDef
+ // into its synthetic wrapper method at all and dies with a bare
+ // `java.lang.AssertionError: assertion failed: method wrapper` during typer, for any input
+ // whatsoever. This mirrors `BoundaryValidatorSpec`; see its header for the same reasoning.
+ // ------------------------------------------------------------------------
private lazy val tb: ToolBox[scala.reflect.runtime.universe.type] = currentMirror.mkToolBox()
- private def inPybuilderPkg(code: String): String =
- s"""package org.apache.texera.amber.pybuilder {
- |
- |$code
- |
- |}""".stripMargin
-
- private def assertToolboxDoesNotCompile(code: String): Unit = {
- intercept[Throwable] {
- // compile only (don’t run); macro expansion happens during compilation
- tb.compile(tb.parse(inPybuilderPkg(code)))
- }
- ()
- }
- // Unicode escapes in *generated* Scala source: must be written as "\\uXXXX" in this test file.
- private def scalaUnicodeEscape(ch: Char): String =
- f"\\\\u${ch.toInt}%04X"
+ private val toolboxHeader =
+ """import org.apache.texera.amber.pybuilder.PythonTemplateBuilder._
+ |import org.apache.texera.amber.pybuilder.PyStringTypes._""".stripMargin
+
+ /** Marker present in every `BoundaryValidator` compile-time abort message. */
+ private val boundaryMarker = "@EncodableStringAnnotation argument #"
+
+ /** The benign outcome: the validator did not abort, only the private access failed. */
+ private val benignMarker = "fromInterpolated"
+
+ /** Compile a self-contained `pyb` snippet as a *block*; it always fails, so return the message. */
+ private def macroError(body: String): String =
+ intercept[ToolBoxError] {
+ tb.compile(tb.parse(s"{\n$toolboxHeader\n$body\n}"))
+ }.getMessage
+
+ private def oneLine(message: String): String =
+ message.linesIterator.map(_.trim).filter(_.nonEmpty).mkString(" | ").take(240)
+
+ /**
+ * `"""` spelled through a normal literal.
+ *
+ * The generated snippets below wrap the Python template in a triple-quoted Scala string so the
+ * neighbour character can be embedded verbatim. `\"` is *not* an escape inside a triple-quoted
+ * string, so the delimiter cannot be written inline in the `s"""..."""` builders below.
+ */
+ private val tripleQuote = "\"\"\""
+
+ /** Literal `${ui}` for the generated snippet - a plain literal, not an interpolation. */
+ private val uiSplice = "${ui}"
+
+ /** Wrap one Python template line as a `pyb` snippet with a direct `EncodableString` argument. */
+ private def pybSnippet(pythonTemplate: String): String =
+ s"""val ui: EncodableString = "x"
+ |pyb$tripleQuote$pythonTemplate$tripleQuote""".stripMargin
+
+ /**
+ * The neighbour character, doubled, as it must appear in Scala source.
+ *
+ * Doubling matters for the two quote characters: `validateCompileTime` runs the unclosed-quote
+ * rule *before* the neighbour rules, so a lone `'` or `"` in the prefix would abort with the
+ * "inside a quoted Python string literal" reason instead of the neighbour reason. A pair leaves
+ * the prefix quote-balanced, so the neighbour rule is what fires. For every other character the
+ * doubling is a no-op. `$` is the one character needing escaping, since it would otherwise open
+ * an interpolation in the generated snippet.
+ */
+ private def neighborPad(ch: Char): String = {
+ val asSource = if (ch == '$') "$$" else ch.toString
+ asSource + asSource
+ }
+
+ /** `pyb"""pre<ch><ch>${ui} post"""` - the character is the arg's immediate left neighbour. */
+ private def leftAdjacentSnippet(ch: Char): String =
+ pybSnippet(s"pre${neighborPad(ch)}$uiSplice post")
+
+ /** `pyb"""pre ${ui}<ch><ch> post"""` - the character is the arg's immediate right neighbour. */
+ private def rightAdjacentSnippet(ch: Char): String =
+ pybSnippet(s"pre $uiSplice${neighborPad(ch)} post")
+
+ /**
+ * The `isBadNeighbor` set over printable ASCII, spelled out rather than derived.
+ *
+ * Deriving the sweep's input from `PythonLexerUtils.isBadNeighbor` would make it
+ * self-referential: shrinking the predicate would silently shrink the sweep instead of failing
+ * it. `PythonLexerUtilsSpec` only *samples* the predicate (`'`, `"`, `a`, `Z`, `0`, `_`, plus
+ * two negatives), so it too stays green when a single character is dropped from the set. The
+ * two sweeps below pin the whole set from the outside - shrinking it reds the bad sweep,
+ * growing it reds the safe sweep.
+ */
+ private val badNeighborChars: Seq[Char] =
+ "\"'0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz".toSeq
+
+ /** Printable ASCII (33..126, so no whitespace) that is *not* in [[badNeighborChars]]. */
+ private val safeNeighborChars: Seq[Char] =
+ (33 to 126).map(_.toChar).filterNot(badNeighborChars.contains)
// ========================================================================
// Rendering basics (plain vs encoded)
@@ -435,38 +509,79 @@ class PythonTemplateBuilderSpec extends AnyFunSuite {
}
test("all isBadNeighbor characters reject direct UI adjacency at compile time (left + right)") {
- val candidates = (33 to 126).map(_.toChar) // printable ASCII, avoids whitespace
- val badChars = candidates.filter(PythonLexerUtils.isBadNeighbor)
-
- // This is intentionally exhaustive over the implementation-defined "bad neighbor" set.
- // We assert only compile success/failure, not the specific error message.
- badChars.zipWithIndex.foreach {
- case (ch, i) =>
- val esc = scalaUnicodeEscape(ch)
-
- val leftAdj =
- s"""
- |import org.apache.texera.amber.pybuilder.PythonTemplateBuilder._
- |import org.apache.texera.amber.pybuilder.PyStringTypes._
- |object UiBadLeft_$i {
- | val ui: EncodableString = "x"
- | val b = pyb\"\"\"pre$esc${'$'}{ui}post\"\"\"
- |}
- |""".stripMargin
-
- val rightAdj =
- s"""
- |import org.apache.texera.amber.pybuilder.PythonTemplateBuilder._
- |import org.apache.texera.amber.pybuilder.PyStringTypes._
- |object UiBadRight_$i {
- | val ui: EncodableString = "x"
- | val b = pyb\"\"\"pre${'$'}{ui}$esc post\"\"\"
- |}
- |""".stripMargin
-
- assertToolboxDoesNotCompile(leftAdj)
- assertToolboxDoesNotCompile(rightAdj)
+ assert(badNeighborChars.size == 65, "the sweep's character set changed unexpectedly")
+
+ val mismatches = List.newBuilder[String]
+ var checked = 0
+
+ badNeighborChars.foreach { ch =>
+ Seq(
+ ("left", leftAdjacentSnippet(ch), s"must not be immediately adjacent to '$ch' on the left"),
+ (
+ "right",
+ rightAdjacentSnippet(ch),
+ s"must not be immediately adjacent to '$ch' on the right"
+ )
+ ).foreach {
+ case (side, snippet, expectedReason) =>
+ checked += 1
+ val message = macroError(snippet)
+ if (!message.contains(boundaryMarker) || !message.contains(expectedReason)) {
+ mismatches += s"$side [$ch] (U+${"%04X".format(ch.toInt)}): ${oneLine(message)}"
+ }
+ }
+ }
+
+ assert(checked == 130, "the sweep did not run every case")
+ val failures = mismatches.result()
+ assert(
+ failures.isEmpty,
+ s"${failures.size} of $checked adjacency cases did not abort with the neighbor reason:\n" +
+ failures.mkString("\n")
+ )
+ }
+
+ test("no safe-neighbour character aborts direct UI adjacency (the sweep discriminates)") {
+ // The counterpart of the sweep above: without this, weakening the neighbour rule to "always
+ // abort" would leave that sweep green. `#` on the left is excluded because the comment rule
+ // legitimately fires first there; the test below pins that case separately.
+ assert(safeNeighborChars.size == 29, "the sweep's character set changed unexpectedly")
+
+ val aborts = List.newBuilder[String]
+ var checked = 0
+
+ safeNeighborChars.foreach { ch =>
+ val sides =
+ if (ch == '#') Seq("right" -> rightAdjacentSnippet(ch))
+ else Seq("left" -> leftAdjacentSnippet(ch), "right" -> rightAdjacentSnippet(ch))
+
+ sides.foreach {
+ case (side, snippet) =>
+ checked += 1
+ val message = macroError(snippet)
+ if (message.contains(boundaryMarker) || !message.contains(benignMarker)) {
+ aborts += s"$side [$ch] (U+${"%04X".format(ch.toInt)}): ${oneLine(message)}"
+ }
+ }
}
+
+ assert(checked == 57, "the sweep did not run every case")
+ val failures = aborts.result()
+ assert(
+ failures.isEmpty,
+ s"${failures.size} of $checked safe-neighbour cases were not benign:\n" +
+ failures.mkString("\n")
+ )
+ }
+
+ test("'#' as a left neighbour aborts for the comment rule, not the neighbour rule") {
+ val message = macroError(leftAdjacentSnippet('#'))
+ assert(message.contains(boundaryMarker), oneLine(message))
+ assert(
+ message.contains("appears after a '#' comment marker on the same line."),
+ oneLine(message)
+ )
+ assert(!message.contains("must not be immediately adjacent"), oneLine(message))
}
// ========================================================================