Copilot commented on code in PR #7864:
URL: https://github.com/apache/texera/pull/7864#discussion_r3837767205
##########
common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/EncodableInspectorSpec.scala:
##########
@@ -189,4 +273,274 @@ class EncodableInspectorSpec extends AnyFunSuite {
|pyb"a $ui b"""".stripMargin
)
}
+
+ // ========================================================================
+ // Probe-macro tests: these read the classifier's *actual* answers rather
than
+ // inferring them from compile-error text, and they can hand the inspector
+ // argument shapes `pyb"..."` cannot produce.
+ //
+ // `flags` is always "ptb=<b> sr=<b> enc=<b> treeEnc=<b>", i.e.
+ // isPythonTemplateBuilderArg / isStringRendererArg /
isDirectEncodableStringArg /
+ // treeHasEncodableString, in that order.
+ // ========================================================================
+
+ // NOTE, so nobody over-reads the coverage these two tests buy:
`isStringRendererArg` has no
+ // caller anywhere in the repo outside this spec (`wrapArg` inlines the same
subtype check).
+ // Its body is therefore production code that only the test harness
executes, which is exactly
+ // why its two lines were never covered before. The predicate is still worth
pinning - `wrapArg`
+ // duplicates its logic - but the coverage it adds is not new *production*
behaviour.
+ test("isStringRendererArg accepts a StringRenderer-typed argument") {
+ assert(
+ evalProbe("""classify(PyLiteralStringRenderer("x"))""") ==
+ "ptb=false sr=true enc=false treeEnc=false"
+ )
+ }
+
+ test("isStringRendererArg rejects a plain String argument") {
+ assert(evalProbe("""classify("x")""") == "ptb=false sr=false enc=false
treeEnc=false")
+ }
+
+ test("isPythonTemplateBuilderArg accepts a PythonTemplateBuilder-typed
argument") {
+ // A PythonTemplateBuilder cannot be *constructed* inside a ToolBox
snippet (its factory is
+ // private[amber]), but a declaration of that type is enough: the probe
macro only inspects
+ // the argument tree, it never evaluates it.
+ assert(
+ evalProbe(
+ """def nested: org.apache.texera.amber.pybuilder.PythonTemplateBuilder
= ???
+ |classify(nested)""".stripMargin
+ ) == "ptb=true sr=false enc=false treeEnc=false"
+ )
+ }
+
+ test("wrapArg keeps an existing EncodableStringRenderer as a cast rather
than re-wrapping it") {
+ // This fixture satisfies priority 1 only (`treeEnc=false`), so it pins
the cast *branch* -
+ // swapping its body with the literal fallback fails here - but NOT the
branch *order*.
+ // The order is pinned by "wrapArg prefers the StringRenderer cast ..."
below.
+ assert(
+ evalProbe("""classify(EncodableStringRenderer("x"))""") ==
+ "ptb=false sr=true enc=true treeEnc=false"
+ )
+ val wrap = evalProbe("""wrapCode(EncodableStringRenderer("x"))""")
+ assert(wrap.contains(".asInstanceOf["), wrap)
+ assert(wrap.endsWith("PythonTemplateBuilder.StringRenderer]"), wrap)
+ assert(!wrap.contains(".toString"), wrap)
+ }
+
+ test("a never-typechecked argument tree is classified non-Encodable by every
predicate") {
+ // Pins the fail-safe default of the `tpe != null` / `argType != null`
guards on lines 131,
+ // 136, 147 and 164: swapping either `&&` so the guard runs second turns
this test into an NPE.
+ //
+ // Honest scope note: `pyb` cannot deliver such a tree - a blackbox
macro's argument trees are
+ // typechecked by construction - so these guards are defensive-only,
reachable in practice only
+ // through a probe like this one. And the *fifth* null guard, `tree.tpe !=
null` on line 126, is
+ // provably redundant rather than merely untested:
`typeHasEncodableString`'s `loop` already
+ // answers false for null on line 79, so deleting it is an equivalent
mutation. No test can (or
+ // should) pretend to pin it.
Review Comment:
These comments hard-code production-file line numbers (e.g., "guards on
lines 131, 136, 147 and 164"). Line numbers will drift as
EncodableInspector.scala changes, making the rationale misleading. Prefer
referencing the specific methods/conditions (e.g.,
`isPythonTemplateBuilderArg`/`isStringRendererArg` null guards, `wrapArg`'s
`argType != null` check) instead of numeric line references.
This issue also appears in the following locations of the same file:
- line 348
- line 405
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
