Copilot commented on code in PR #6810:
URL: https://github.com/apache/texera/pull/6810#discussion_r3634622208
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/rangeSlider/RangeSliderOpDescSpec.scala:
##########
@@ -74,4 +77,15 @@ class RangeSliderOpDescSpec extends AnyFlatSpec with
Matchers {
r.yAxis shouldBe "sales"
r.duplicateType shouldBe RangeSliderHandleDuplicateFunction.MEAN
}
+
+ "RangeSliderOpDesc @JsonSchemaInject" should
+ "constrain the aggregated Y-axis to numeric and leave X-axis
unconstrained" in {
+ // The rule key must be an actual @JsonProperty name; a key of "value" (no
such
+ // field) matches nothing, so no numeric constraint reaches the column
pickers.
+ val rules = objectMapper
+
.readTree(classOf[RangeSliderOpDesc].getAnnotation(classOf[JsonSchemaInject]).json())
+ .path("attributeTypeRules")
+ rules.fieldNames().asScala.toSet shouldBe Set("Y-axis")
+ rules.path("Y-axis").path("enum").toString should include("double")
Review Comment:
The test only checks that the enum JSON contains "double". That would still
pass if the numeric constraint accidentally dropped "integer" or "long",
weakening the regression signal. It also dereferences the annotation without
asserting it exists, which can turn failures into a less-informative NPE.
Consider asserting the exact enum set and that `@JsonSchemaInject` is
present (similar to DumbbellDotConfigSpec).
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/quiverPlot/QuiverPlotOpDescSpec.scala:
##########
@@ -96,4 +98,17 @@ class QuiverPlotOpDescSpec extends AnyFlatSpec with Matchers
{
q.u shouldBe "vu"
q.v shouldBe "vv"
}
+
+ "QuiverPlotOpDesc @JsonSchemaInject" should
+ "constrain the real coordinate fields (x/y/u/v) to numeric" in {
+ // The rule keys must be actual @JsonProperty names; a key of "value" (no
such
+ // field) matches nothing, so no numeric constraint reaches the column
pickers.
+ val rules = objectMapper
+
.readTree(classOf[QuiverPlotOpDesc].getAnnotation(classOf[JsonSchemaInject]).json())
+ .path("attributeTypeRules")
+ rules.fieldNames().asScala.toSet shouldBe Set("x", "y", "u", "v")
+ rules.fieldNames().asScala.foreach { f =>
+ rules.path(f).path("enum").toString should include("double")
+ }
Review Comment:
This regression test currently asserts only that each enum JSON contains
"double". If "integer" or "long" were accidentally removed from the constraint,
the test would still pass. Also, consider asserting `@JsonSchemaInject` is
present before reading it to avoid an NPE masking the real failure.
Suggest asserting the exact enum set for each field (integer/long/double),
similar to DumbbellDotConfigSpec.
--
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]
