Copilot commented on code in PR #4809: URL: https://github.com/apache/texera/pull/4809#discussion_r3177575463
########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/volcanoPlot/VolcanoPlotOpDescSpec.scala: ########## @@ -0,0 +1,81 @@ +/* + * 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.operator.visualization.volcanoPlot + +import org.apache.texera.amber.core.tuple.AttributeType +import org.apache.texera.amber.operator.metadata.OperatorGroupConstants +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class VolcanoPlotOpDescSpec extends AnyFlatSpec with Matchers { + + private def configured: VolcanoPlotOpDesc = { + val op = new VolcanoPlotOpDesc + op.effectColumn = "log2fc" + op.pvalueColumn = "pvalue" + op + } + + "VolcanoPlotOpDesc.operatorInfo" should "advertise the user-friendly name and Scientific group" in { + val info = (new VolcanoPlotOpDesc).operatorInfo + info.userFriendlyName shouldBe "Volcano Plot" + info.operatorGroupName shouldBe OperatorGroupConstants.VISUALIZATION_SCIENTIFIC_GROUP + info.operatorDescription should include("statistical") + } + + it should "expose exactly one output port wired through forVisualization" in { + (new VolcanoPlotOpDesc).operatorInfo.outputPorts should have length 1 + } + + "VolcanoPlotOpDesc.getOutputSchemas" should "return a single-port schema with an html-content STRING column" in { + val op = configured + val schemas = op.getOutputSchemas(Map.empty) + schemas should have size 1 + val (portId, schema) = schemas.head + portId shouldBe op.operatorInfo.outputPorts.head.id + schema.getAttributes should have length 1 + schema.getAttributes.head.getName shouldBe "html-content" + schema.getAttributes.head.getType shouldBe AttributeType.STRING + } + + "VolcanoPlotOpDesc.generatePythonCode" should "render a UDFTableOperator source that decodes both column references" in { + // EncodableString fields are NOT emitted as literal column names — the + // pyb macro wraps them in `self.decode_python_template.decode("<base64>")` + // calls so the column name is resolved at runtime. Verify the structure + // (class + import + decode site count) instead of substring matches. + val code = configured.generatePythonCode() + code should include("class ProcessTableOperator(UDFTableOperator)") + code should include("plotly.express") + code should include("-log10(pvalue)") + val decodeOccurrences = "decode_python_template".r.findAllIn(code).length + decodeOccurrences should be >= 2 + } + + it should "still render code when required fields are empty (no assert guard, current behavior)" in { + // Pin: unlike FunnelPlot / ImageVisualizer, VolcanoPlotOpDesc does not + // assert on its required fields inside generatePythonCode. An empty + // configuration therefore renders weird (but syntactically valid) Python + // that references an empty string. Documenting the divergence so a + // future tightening that adds the assert breaks this spec deliberately. + val op = new VolcanoPlotOpDesc + val code = op.generatePythonCode() + code should include("class ProcessTableOperator(UDFTableOperator)") + } Review Comment: This spec deliberately codifies the current lack of validation for `effectColumn`/`pvalueColumn`, even though both fields are marked required in `VolcanoPlotOpDesc`. A future fix that rejects empty required fields would fail this test, so it ends up protecting buggy behavior instead of the operator's contract. ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/bulletChart/BulletChartOpDescSpec.scala: ########## @@ -0,0 +1,102 @@ +/* + * 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.operator.visualization.bulletChart + +import org.apache.texera.amber.core.tuple.AttributeType +import org.apache.texera.amber.operator.metadata.OperatorGroupConstants +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.util +import java.util.{List => JList} + +class BulletChartOpDescSpec extends AnyFlatSpec with Matchers { + + private def configured: BulletChartOpDesc = { + val op = new BulletChartOpDesc + op.value = "actualValue" + op.deltaReference = "100" + op + } + + "BulletChartOpDesc.operatorInfo" should "advertise the user-friendly name and Financial group" in { + val info = (new BulletChartOpDesc).operatorInfo + info.userFriendlyName shouldBe "Bullet Chart" + info.operatorGroupName shouldBe OperatorGroupConstants.VISUALIZATION_FINANCIAL_GROUP + info.operatorDescription should include("Bullet Chart") + } + + it should "expose exactly one output port wired through forVisualization" in { + (new BulletChartOpDesc).operatorInfo.outputPorts should have length 1 + } + + "BulletChartOpDesc.getOutputSchemas" should "return a single-port schema with an html-content STRING column" in { + val op = configured + val schemas = op.getOutputSchemas(Map.empty) + schemas should have size 1 + val (portId, schema) = schemas.head + portId shouldBe op.operatorInfo.outputPorts.head.id + schema.getAttributes should have length 1 + schema.getAttributes.head.getName shouldBe "html-content" + schema.getAttributes.head.getType shouldBe AttributeType.STRING + } + + "BulletChartOpDesc.generatePythonCode" should "render Python source with a runtime decode site for the value column" in { + // EncodableString fields are NOT emitted as literal strings — the pyb + // macro wraps them in `self.decode_python_template.decode("<base64>")` + // calls. The rendered source must reference the decoder symbol at least + // for `value` and `deltaReference`. + val code = configured.generatePythonCode() + code should include("plotly.graph_objects") + val decodeOccurrences = "decode_python_template".r.findAllIn(code).length + decodeOccurrences should be >= 2 + } + + it should "default to an empty steps list when none are configured" in { + val code = configured.generatePythonCode() + code should include("[]") + } + + it should "include each configured step's start/end JSON keys with extra decode sites" in { + val op = configured + val steps: JList[BulletChartStepDefinition] = new util.ArrayList[BulletChartStepDefinition]() + steps.add(new BulletChartStepDefinition("0", "50")) + steps.add(new BulletChartStepDefinition("50", "100")) + op.steps = steps + val code = op.generatePythonCode() + code should include("\"start\":") + code should include("\"end\":") + // Two steps × 2 EncodableString fields each = 4 extra decode sites on + // top of the value/deltaReference decodes from the base configuration. + val baseDecodes = "decode_python_template".r.findAllIn(configured.generatePythonCode()).length + val withSteps = "decode_python_template".r.findAllIn(code).length + withSteps shouldBe baseDecodes + 4 + } + + it should "render a code block even with the default empty configuration (no assert guard)" in { + // Pin: BulletChartOpDesc, unlike FunnelPlot / ImageVisualizer, has no + // assert guards inside generatePythonCode. Empty defaults still produce + // valid Python source. Pinning so a future tightening that adds asserts + // breaks this spec deliberately. + val op = new BulletChartOpDesc Review Comment: This spec intentionally enshrines the current acceptance of empty `value`/`deltaReference`, even though both properties are marked required on `BulletChartOpDesc`. If the operator is later fixed to validate missing required fields, this test will fail and block that improvement. ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/bulletChart/BulletChartOpDescSpec.scala: ########## @@ -0,0 +1,102 @@ +/* + * 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.operator.visualization.bulletChart + +import org.apache.texera.amber.core.tuple.AttributeType +import org.apache.texera.amber.operator.metadata.OperatorGroupConstants +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.util +import java.util.{List => JList} + +class BulletChartOpDescSpec extends AnyFlatSpec with Matchers { + + private def configured: BulletChartOpDesc = { + val op = new BulletChartOpDesc + op.value = "actualValue" + op.deltaReference = "100" + op + } + + "BulletChartOpDesc.operatorInfo" should "advertise the user-friendly name and Financial group" in { + val info = (new BulletChartOpDesc).operatorInfo + info.userFriendlyName shouldBe "Bullet Chart" + info.operatorGroupName shouldBe OperatorGroupConstants.VISUALIZATION_FINANCIAL_GROUP + info.operatorDescription should include("Bullet Chart") + } + + it should "expose exactly one output port wired through forVisualization" in { + (new BulletChartOpDesc).operatorInfo.outputPorts should have length 1 + } + + "BulletChartOpDesc.getOutputSchemas" should "return a single-port schema with an html-content STRING column" in { + val op = configured + val schemas = op.getOutputSchemas(Map.empty) + schemas should have size 1 + val (portId, schema) = schemas.head + portId shouldBe op.operatorInfo.outputPorts.head.id + schema.getAttributes should have length 1 + schema.getAttributes.head.getName shouldBe "html-content" + schema.getAttributes.head.getType shouldBe AttributeType.STRING + } + + "BulletChartOpDesc.generatePythonCode" should "render Python source with a runtime decode site for the value column" in { + // EncodableString fields are NOT emitted as literal strings — the pyb + // macro wraps them in `self.decode_python_template.decode("<base64>")` + // calls. The rendered source must reference the decoder symbol at least + // for `value` and `deltaReference`. + val code = configured.generatePythonCode() + code should include("plotly.graph_objects") + val decodeOccurrences = "decode_python_template".r.findAllIn(code).length + decodeOccurrences should be >= 2 + } + + it should "default to an empty steps list when none are configured" in { + val code = configured.generatePythonCode() + code should include("[]") Review Comment: This assertion is too weak to verify that the generated `steps_data = ...` literal defaults to an empty list. The bullet-chart template already contains several unrelated `[]` literals (`colors`, `valid_steps`, `step_errors`, `steps_list`, `html_chunks`), so this test will still pass if the default `stepsStr` output regresses. ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/ImageViz/ImageVisualizerOpDescSpec.scala: ########## @@ -19,18 +19,61 @@ package org.apache.texera.amber.operator.visualization.ImageViz +import org.apache.texera.amber.core.tuple.AttributeType +import org.apache.texera.amber.operator.metadata.OperatorGroupConstants import org.scalatest.BeforeAndAfter import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers -class ImageVisualizerOpDescSpec extends AnyFlatSpec with BeforeAndAfter { +class ImageVisualizerOpDescSpec extends AnyFlatSpec with BeforeAndAfter with Matchers { var opDesc: ImageVisualizerOpDesc = _ before { opDesc = new ImageVisualizerOpDesc() } it should "throw assertion error if BinaryContent is empty" in { + // Pin: `binaryContent` is declared `var binaryContent: EncodableString = _` + // — an uninitialized reference field defaults to null, so the + // `assert(binaryContent.nonEmpty)` inside `createBinaryData` reaches + // `null.nonEmpty` and throws NullPointerException before the assert + // message can fire. Other OpDescs that initialize their fields to `""` + // hit the assert path and raise AssertionError instead. assertThrows[NullPointerException] { Review Comment: This test locks in the current NullPointerException from dereferencing an uninitialized required field. Because `binaryContent` is declared `@JsonProperty(required = true)`, the intended contract is that missing configuration should be rejected in a controlled way; pinning the NPE here will make a future validation fix look like a regression. -- 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]
