mengw15 commented on code in PR #5897: URL: https://github.com/apache/texera/pull/5897#discussion_r3457936733
########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/radarPlot/RadarPlotOpDescSpec.scala: ########## @@ -0,0 +1,115 @@ +/* + * 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.radarPlot + +import org.apache.texera.amber.core.tuple.{AttributeType, Schema} +import org.apache.texera.amber.operator.LogicalOp +import org.apache.texera.amber.operator.metadata.OperatorGroupConstants +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.nio.charset.StandardCharsets +import java.util.Base64 + +class RadarPlotOpDescSpec extends AnyFlatSpec with Matchers { + + private def b64(s: String): String = + Base64.getEncoder.encodeToString(s.getBytes(StandardCharsets.UTF_8)) + + // EncodableString axes are always base64-wrapped in .encode mode + // (self.decode_python_template('<base64>')), so assert on the base64 form only rather than + // the raw column name, which could appear in the generated Python for unrelated reasons. + private def carries(output: String, name: String): Boolean = + output.contains(b64(name)) + + "RadarPlotOpDesc.operatorInfo" should + "advertise the name and Scientific visualization group with a 1-in/1-out shape" in { + val info = (new RadarPlotOpDesc).operatorInfo + info.userFriendlyName shouldBe "Radar Plot" + info.operatorDescription shouldBe "View the result in a radar plot." + info.operatorGroupName shouldBe OperatorGroupConstants.VISUALIZATION_SCIENTIFIC_GROUP + info.inputPorts should have length 1 + info.outputPorts should have length 1 + } + + "RadarPlotOpDesc" should "default the boolean flags to true and the optional columns to empty" in { + val d = new RadarPlotOpDesc + d.maxNormalize shouldBe true + d.fillTrace shouldBe true + d.showMarkers shouldBe true + d.showLegend shouldBe true + d.traceNameAttribute shouldBe "" + d.traceColorAttribute shouldBe "" + d.selectedAttributes shouldBe null + d.linePattern shouldBe null + } + + "RadarPlotOpDesc.getOutputSchemas" should + "produce a single html-content STRING column keyed by the declared output port" in { + val op = new RadarPlotOpDesc + op.getOutputSchemas(Map.empty) shouldBe Map( + op.operatorInfo.outputPorts.head.id -> Schema().add("html-content", AttributeType.STRING) + ) + } + + "RadarPlotOpDesc.generatePythonCode" should + "require linePattern to be set (it is dereferenced without a null guard)" in { Review Comment: This test pins a `NullPointerException` from `generatePythonCode` as the contract — combined with the defaults test above (`d.linePattern shouldBe null`), it's documenting that a default-constructed `RadarPlotOpDesc` will NPE if `generatePythonCode()` is called without first setting `linePattern`. That's a real bug in `RadarPlotOpDesc` (NPE on a public method with default state), and pinning it via `intercept[NullPointerException]` turns the bug into the spec — a future contributor who fixes the null guard would have to delete this test. Worth surfacing either as a follow-up issue (fix the dereference: either `require(linePattern != null, "...")` for a clearer error, or default `linePattern` to a sensible value) or rephrasing the test to assert the contract you actually want (e.g. `IllegalArgumentException` from a guarded `require`). As-is, the test's "should require linePattern to be set" phrasing reads like a feature, but the behavior is unguarded-NPE-on-uninitialized-state. Not blocking the test-coverage win here, just flagging. -- 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]
