test(pybuilder): make the bad-neighbour boundary sweep actually compile its snippets

[aglinxinyuan]

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:

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)

[github-actions]

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

• Contributors with relevant context: @Yicong-Huang
  You can notify them by mentioning @Yicong-Huang in a comment.

[Copilot]

🟢 Approval recommended

The change is isolated to tests, aligns with existing ToolBox-based patterns in the module, and the only feedback is minor spelling consistency nits in new test text.

Pull request overview

This PR fixes a vacuous/exhaustive “bad-neighbour” boundary sweep in PythonTemplateBuilderSpec by changing the runtime ToolBox harness so the pyb macro actually expands (and therefore can meaningfully distinguish boundary aborts from the expected post-expansion fromInterpolated access failure).

Changes:

• Replaced the old package ... { ... } ToolBox compilation approach with a block-based compilation harness that reaches macro expansion and captures ToolBoxError messages.
• Rewrote the exhaustive bad-neighbor sweep to assert on the boundary abort marker plus the specific left/right adjacency reason per character.
• Added discriminating coverage for “safe” neighbor characters (benign outcome) and a dedicated #-left-neighbor test to pin the comment-rule precedence.

File summaries

File	Description
common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala	Reworks ToolBox-based macro-expansion testing and adds non-vacuous exhaustive boundary sweeps with message-based assertions.

Review details

Suppressed comments (3)

common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala:547

• These new test names/comments use “neighbour”, while the surrounding APIs/tests use “neighbor” (e.g., isBadNeighbor). Aligning the spelling makes grep/search and terminology consistent across the suite.

  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.

common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala:572

• Spelling consistency: update “safe-neighbour” to “safe-neighbor” (matches isBadNeighbor / BoundaryValidator wording).

      s"${failures.size} of $checked safe-neighbour cases were not benign:\n" +

common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala:577

• Spelling consistency: switch “neighbour” -> “neighbor” in this test name to match the rest of the pybuilder terminology (isBadNeighbor, badLeftNeighbor, etc.).

  test("'#' as a left neighbour aborts for the comment rule, not the neighbour rule") {

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.02%. Comparing base (1cbe857) to head (8896f51).

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8405      +/-   ##
============================================
- Coverage     94.03%   94.02%   -0.01%     
+ Complexity     4821     4820       -1     
============================================
  Files          1204     1204              
  Lines         48991    48991              
  Branches       5956     5956              
============================================
- Hits          46067    46066       -1     
  Misses         1458     1458              
- Partials       1466     1467       +1     

Flag	Coverage Δ		*Carryforward flag
access-control-service	81.00% <ø> (ø)
agent-service	99.32% <ø> (ø)		Carriedforward from 1cbe857
amber	89.91% <ø> (-0.01%)	⬇️
computing-unit-managing-service	73.67% <ø> (ø)
config-service	87.12% <ø> (ø)
file-service	87.91% <ø> (ø)
frontend	96.79% <ø> (ø)		Carriedforward from 1cbe857
notebook-migration-service	83.57% <ø> (ø)
pyamber	98.47% <ø> (ø)		Carriedforward from 1cbe857
workflow-compiling-service	77.19% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 2 worse · ⚪ 11 noise (<±5%) · 0 without baseline

Compared against main 1cbe857 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 · Run

	config	throughput	MB/s	latency	max Δ latest / 7d
🔴	bs=10 sw=10 sl=64	512	0.313	18,452/27,763/27,763 us	🔴 -7.9% / 🔴 +73.7%
🟢	bs=100 sw=10 sl=64	1,154	0.705	84,856/123,368/123,368 us	🟢 -7.7% / 🟢 +18.3%
⚪	bs=1000 sw=10 sl=64	1,393	0.85	719,287/776,090/776,090 us	⚪ within ±5% / 🟢 +38.4%

Baseline details

Latest main 1cbe857 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	512 tuples/sec	556 tuples/sec	755.36 tuples/sec	-7.9%	-32.2%
bs=10 sw=10 sl=64	MB/s	0.313 MB/s	0.339 MB/s	0.461 MB/s	-7.7%	-32.1%
bs=10 sw=10 sl=64	p50	18,452 us	17,696 us	12,938 us	+4.3%	+42.6%
bs=10 sw=10 sl=64	p95	27,763 us	28,556 us	15,980 us	-2.8%	+73.7%
bs=10 sw=10 sl=64	p99	27,763 us	28,556 us	19,233 us	-2.8%	+44.4%
bs=100 sw=10 sl=64	throughput	1,154 tuples/sec	1,135 tuples/sec	976.3 tuples/sec	+1.7%	+18.2%
bs=100 sw=10 sl=64	MB/s	0.705 MB/s	0.693 MB/s	0.596 MB/s	+1.7%	+18.3%
bs=100 sw=10 sl=64	p50	84,856 us	86,599 us	102,340 us	-2.0%	-17.1%
bs=100 sw=10 sl=64	p95	123,368 us	133,674 us	109,262 us	-7.7%	+12.9%
bs=100 sw=10 sl=64	p99	123,368 us	133,674 us	118,827 us	-7.7%	+3.8%
bs=1000 sw=10 sl=64	throughput	1,393 tuples/sec	1,395 tuples/sec	1,006 tuples/sec	-0.1%	+38.4%
bs=1000 sw=10 sl=64	MB/s	0.85 MB/s	0.851 MB/s	0.614 MB/s	-0.1%	+38.4%
bs=1000 sw=10 sl=64	p50	719,287 us	715,813 us	999,855 us	+0.5%	-28.1%
bs=1000 sw=10 sl=64	p95	776,090 us	792,355 us	1,042,833 us	-2.1%	-25.6%
bs=1000 sw=10 sl=64	p99	776,090 us	792,355 us	1,070,722 us	-2.1%	-27.5%

Raw 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,390.36,200,128000,512,0.313,18451.57,27762.89,27762.89
1,100,10,64,20,1732.65,2000,1280000,1154,0.705,84856.35,123367.61,123367.61
2,1000,10,64,20,14354.38,20000,12800000,1393,0.850,719287.09,776089.87,776089.87