Copilot commented on code in PR #5697: URL: https://github.com/apache/texera/pull/5697#discussion_r3408752900
########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/tablesChart/TablesConfigSpec.scala: ########## @@ -0,0 +1,99 @@ +/* + * 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.tablesChart + +import com.fasterxml.jackson.annotation.JsonProperty +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec + +import javax.validation.constraints.NotNull + +class TablesConfigSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Default state + // --------------------------------------------------------------------------- + + "TablesConfig" should "default attributeName to the empty string" in { + val c = new TablesConfig + assert(c.attributeName == "") + } + + // --------------------------------------------------------------------------- + // Mutability — the @JsonProperty bag is `var`-based by design + // --------------------------------------------------------------------------- + + it should "allow attributeName to be assigned post-construction" in { + val c = new TablesConfig + c.attributeName = "col-a" + assert(c.attributeName == "col-a") + } + + // --------------------------------------------------------------------------- + // JSON round-trip preserves the field + // --------------------------------------------------------------------------- + + "TablesConfig JSON round-trip" should + "serialize and deserialize attributeName unchanged" in { + val original = new TablesConfig + original.attributeName = "my-attr" + val json = objectMapper.writeValueAsString(original) + val restored = objectMapper.readValue(json, classOf[TablesConfig]) + assert(restored.attributeName == "my-attr") + } + + it should "round-trip the default empty attributeName" in { + val original = new TablesConfig + val restored = objectMapper.readValue( + objectMapper.writeValueAsString(original), + classOf[TablesConfig] + ) + assert(restored.attributeName == "") + } + + // --------------------------------------------------------------------------- + // Independent instances — no static-field leakage + // --------------------------------------------------------------------------- + + it should "construct two independent instances (no static state shared)" in { + val a = new TablesConfig + val b = new TablesConfig + a.attributeName = "first" + assert(b.attributeName == "", "second instance must not see first instance's mutation") + } + + // --------------------------------------------------------------------------- + // Annotations — verified via reflection (the Jackson + validation + // contract is what consumers actually depend on) + // --------------------------------------------------------------------------- + + "TablesConfig#attributeName" should "carry @JsonProperty(required = true)" in { + val field = classOf[TablesConfig].getDeclaredField("attributeName") + val jp = field.getAnnotation(classOf[JsonProperty]) + assert(jp != null, "attributeName must carry @JsonProperty") + assert(jp.required, "attributeName must be marked required") + } + + it should "carry @NotNull (jakarta validation contract)" in { + val field = classOf[TablesConfig].getDeclaredField("attributeName") + val notNull = field.getAnnotation(classOf[NotNull]) + assert(notNull != null, "attributeName must carry @NotNull for jakarta validation") + } +} Review Comment: This spec refers to “jakarta validation”, but the codebase is using `javax.validation.constraints.NotNull` (both in the source class and here). Please update the test description/message to avoid confusion, and consider also pinning `@AutofillAttributeName` since `TablesConfig.attributeName` carries it in production code. ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/tablesChart/TablesConfigSpec.scala: ########## @@ -0,0 +1,99 @@ +/* + * 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.tablesChart + +import com.fasterxml.jackson.annotation.JsonProperty +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec + +import javax.validation.constraints.NotNull + Review Comment: The suggested annotation assertion for `@AutofillAttributeName` requires importing the annotation type here (matching other config specs). ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/nestedTable/NestedTableConfigSpec.scala: ########## @@ -0,0 +1,132 @@ +/* + * 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.nestedTable + +import com.fasterxml.jackson.annotation.JsonProperty +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec + +class NestedTableConfigSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Default state + // --------------------------------------------------------------------------- + + "NestedTableConfig" should "default every field to the empty string" in { + val c = new NestedTableConfig + assert(c.attributeGroup == "") + assert(c.originalName == "") + assert(c.newName == "") + } + + // --------------------------------------------------------------------------- + // Mutability + // --------------------------------------------------------------------------- + + it should "allow every field to be assigned post-construction" in { + val c = new NestedTableConfig + c.attributeGroup = "g" + c.originalName = "orig" + c.newName = "renamed" + assert(c.attributeGroup == "g") + assert(c.originalName == "orig") + assert(c.newName == "renamed") + } + + // --------------------------------------------------------------------------- + // JSON round-trip + // --------------------------------------------------------------------------- + + "NestedTableConfig JSON round-trip" should "preserve all three fields" in { + val original = new NestedTableConfig + original.attributeGroup = "group-1" + original.originalName = "src-name" + original.newName = "dst-name" + val json = objectMapper.writeValueAsString(original) + val restored = objectMapper.readValue(json, classOf[NestedTableConfig]) + assert(restored.attributeGroup == "group-1") + assert(restored.originalName == "src-name") + assert(restored.newName == "dst-name") + } + + it should + "serialize newName under the JSON key `name` (per @JsonProperty(value = \"name\"))" in { + // The wire-key for `newName` is `name`, not `newName` — a regression + // that drifted to `newName` would silently break every workflow JSON + // that carries this config. Pin the wire-key explicitly. + val c = new NestedTableConfig + c.newName = "renamed" + val json = objectMapper.writeValueAsString(c) + assert( + json.contains("\"name\":\"renamed\""), + s"expected 'name':'renamed' wire-key in JSON, got: $json" + ) + assert( + !json.contains("\"newName\""), + s"the field name 'newName' must NOT appear as a JSON key, got: $json" + ) + } Review Comment: These assertions depend on Jackson’s exact string formatting (e.g., no spaces after `:`), which can make the test brittle if `objectMapper` settings change. Prefer inspecting a parsed JSON tree to assert the presence/absence of wire keys and their values. ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/dumbbellPlot/DumbbellDotConfigSpec.scala: ########## @@ -0,0 +1,128 @@ +/* + * 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.dumbbellPlot + +import com.fasterxml.jackson.annotation.JsonProperty +import com.kjetland.jackson.jsonSchema.annotations.JsonSchemaInject +import org.apache.texera.amber.operator.metadata.annotations.AutofillAttributeName +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec + +import javax.validation.constraints.NotNull + +class DumbbellDotConfigSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Default state + // --------------------------------------------------------------------------- + + "DumbbellDotConfig" should "default dotValue to the empty string" in { + val c = new DumbbellDotConfig + assert(c.dotValue == "") + } + + // --------------------------------------------------------------------------- + // Mutability + // --------------------------------------------------------------------------- + + it should "allow dotValue to be assigned post-construction" in { + val c = new DumbbellDotConfig + c.dotValue = "numeric-col" + assert(c.dotValue == "numeric-col") + } + + // --------------------------------------------------------------------------- + // JSON round-trip — verify both the Scala field name AND the wire key + // --------------------------------------------------------------------------- + + "DumbbellDotConfig JSON round-trip" should + "preserve dotValue via the wire-key `dot` (per @JsonProperty(value = \"dot\"))" in { + val original = new DumbbellDotConfig + original.dotValue = "amount" + val json = objectMapper.writeValueAsString(original) + assert( + json.contains("\"dot\":\"amount\""), + s"expected 'dot':'amount' wire-key in JSON, got: $json" + ) + val restored = objectMapper.readValue(json, classOf[DumbbellDotConfig]) + assert(restored.dotValue == "amount") + } Review Comment: This assertion depends on Jackson’s exact string formatting (no spaces, ordering, etc.), which can make the test brittle if `objectMapper` configuration changes. Parse the JSON and assert on the `dot` field directly. ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/dumbbellPlot/DumbbellDotConfigSpec.scala: ########## @@ -0,0 +1,128 @@ +/* + * 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.dumbbellPlot + +import com.fasterxml.jackson.annotation.JsonProperty +import com.kjetland.jackson.jsonSchema.annotations.JsonSchemaInject +import org.apache.texera.amber.operator.metadata.annotations.AutofillAttributeName +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec + +import javax.validation.constraints.NotNull + +class DumbbellDotConfigSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Default state + // --------------------------------------------------------------------------- + + "DumbbellDotConfig" should "default dotValue to the empty string" in { + val c = new DumbbellDotConfig + assert(c.dotValue == "") + } + + // --------------------------------------------------------------------------- + // Mutability + // --------------------------------------------------------------------------- + + it should "allow dotValue to be assigned post-construction" in { + val c = new DumbbellDotConfig + c.dotValue = "numeric-col" + assert(c.dotValue == "numeric-col") + } + + // --------------------------------------------------------------------------- + // JSON round-trip — verify both the Scala field name AND the wire key + // --------------------------------------------------------------------------- + + "DumbbellDotConfig JSON round-trip" should + "preserve dotValue via the wire-key `dot` (per @JsonProperty(value = \"dot\"))" in { + val original = new DumbbellDotConfig + original.dotValue = "amount" + val json = objectMapper.writeValueAsString(original) + assert( + json.contains("\"dot\":\"amount\""), + s"expected 'dot':'amount' wire-key in JSON, got: $json" + ) + val restored = objectMapper.readValue(json, classOf[DumbbellDotConfig]) + assert(restored.dotValue == "amount") + } + + it should "deserialize from the wire-key `dot` back into dotValue" in { + val json = """{"dot":"amount"}""" + val restored = objectMapper.readValue(json, classOf[DumbbellDotConfig]) + assert(restored.dotValue == "amount") + } + + // --------------------------------------------------------------------------- + // Annotations + // --------------------------------------------------------------------------- + + "DumbbellDotConfig#dotValue" should + "carry @JsonProperty(value = 'dot', required = true)" in { + val jp = classOf[DumbbellDotConfig] + .getDeclaredField("dotValue") + .getAnnotation(classOf[JsonProperty]) + assert(jp != null) + val actualValue = jp.value + assert(actualValue == "dot", s"expected value='dot', got: '$actualValue'") + assert(jp.required, "dotValue must be marked required") + } + + it should "carry @NotNull (jakarta validation contract)" in { + val notNull = classOf[DumbbellDotConfig] + .getDeclaredField("dotValue") + .getAnnotation(classOf[NotNull]) + assert(notNull != null, "dotValue must carry @NotNull for jakarta validation") + } Review Comment: This test refers to “jakarta validation”, but the codebase is using `javax.validation.constraints.NotNull` (including in `DumbbellDotConfig`). Update the wording to match the actual validation API to avoid confusion during future migrations. -- 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]
