stevedlawrence commented on code in PR #1579:
URL: https://github.com/apache/daffodil/pull/1579#discussion_r2454828980


##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -80,35 +80,25 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
     // element event to be in the infoset (for round-tripping from parse). 
It's value will
     // get recomputed, but it can appear with a stale value (left-over from 
parse)
     // in the arriving infoset for unparse.
-    //
-    val optDefaultBranch = {
-      val optEmpty: Option[Term] = {
-        val emptyBranches = groupMembers.filter { gm =>
-          val ies = gm.identifyingEventsForChoiceBranch
-          ies.pnes.isEmpty // empty event list makes it the default, not 
simply isOpen
-        }
-        if (emptyBranches.length > 1)
-          SDW(
-            WarnID.MultipleChoiceBranches,
-            "Multiple choice branches with no required elements detected and 
the infoset does not specify a branch, selecting the first branch for unparsing"
-          )
-        emptyBranches.headOption
-      }
-      val optOpen: Option[Term] =
-        optEmpty.orElse {
-          groupMembers.find { gm =>
-            val ies = gm.identifyingEventsForChoiceBranch
-            ies.isOpen // first open one is used if there is no branch that 
has empty event list. (test ptg_1u)
-          }
-        }
-      val optDefault: Option[Term] =
-        optOpen.orElse {
-          groupMembers.find {
-            _.canUnparseIfHidden
-          } // optional, defaultable, OVC, or IVC
-        }
-      optDefault
+    // Rather than defaulting to the first empty branch, we instead default to 
the first
+    // empty, open or defaultable branch.

Review Comment:
   Can you expand these comments to explain what these cases represent? For 
example, what is an "empty" branch? Is that just a branch that literally has no 
elements in it, like just asserts or something? What is an open branch? A 
branch that is made up of entirely optional elements?



##########
daffodil-test/src/test/resources/org/apache/daffodil/section15/choice_groups/choice-unparse2.tdml:
##########
@@ -123,7 +124,6 @@
                 </xs:complexType>
               </xs:element>
             </xs:sequence>
-            <xs:sequence dfdl:hiddenGroupRef="ex:PI_false" />

Review Comment:
   Does order of these branches matter? Feels like it shouldn't.
   
   This choice has two branches, one representing PI_false and one representing 
PI_true.
   
   The PI_false branch has a hiddenGroupRef, so is entirely hidden and could be 
a default branch (assuming it is unparsable, which it is in our case).
   
   The PI_true branch starts with a hidden group ref, but that is followed a 
non-optional "rag" array. So the PI_true branch has at least one required "rag" 
element. Meaning we should never unparse this branch unless we see a "rag" 
start element event, and so it should not be considered for being used as the 
default branch.
   
   So of the two branches, only one can actually be a default branch so order 
shouldn't matter for this particular test. If the new logic could pick the 
PI_true branch as the default branch then I think it indicates we have a bug 
somewhere?



##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -80,35 +80,25 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
     // element event to be in the infoset (for round-tripping from parse). 
It's value will
     // get recomputed, but it can appear with a stale value (left-over from 
parse)
     // in the arriving infoset for unparse.
-    //
-    val optDefaultBranch = {
-      val optEmpty: Option[Term] = {
-        val emptyBranches = groupMembers.filter { gm =>
-          val ies = gm.identifyingEventsForChoiceBranch
-          ies.pnes.isEmpty // empty event list makes it the default, not 
simply isOpen
-        }
-        if (emptyBranches.length > 1)
-          SDW(
-            WarnID.MultipleChoiceBranches,
-            "Multiple choice branches with no required elements detected and 
the infoset does not specify a branch, selecting the first branch for unparsing"
-          )
-        emptyBranches.headOption
-      }
-      val optOpen: Option[Term] =
-        optEmpty.orElse {
-          groupMembers.find { gm =>
-            val ies = gm.identifyingEventsForChoiceBranch
-            ies.isOpen // first open one is used if there is no branch that 
has empty event list. (test ptg_1u)
-          }
-        }
-      val optDefault: Option[Term] =
-        optOpen.orElse {
-          groupMembers.find {
-            _.canUnparseIfHidden
-          } // optional, defaultable, OVC, or IVC
-        }
-      optDefault
+    // Rather than defaulting to the first empty branch, we instead default to 
the first
+    // empty, open or defaultable branch.
+    val emptyOpenOrDefault = groupMembers.filter {
+      case gm if gm.identifyingEventsForChoiceBranch.pnes.isEmpty => true
+      case gm if gm.identifyingEventsForChoiceBranch.isOpen => true
+      case gm if gm.canUnparseIfHidden => true
+      case _ => false
+    }
+    if (
+      emptyOpenOrDefault.length > 1 && emptyOpenOrDefault.forall(
+        _.identifyingEventsForChoiceBranch.pnes.isEmpty
+      )
+    ) {
+      SDW(
+        WarnID.MultipleChoiceBranches,
+        "Multiple choice branches with no required elements detected and the 
infoset does not specify a branch, selecting the first branch for unparsing"
+      )

Review Comment:
   I wonder if we should remove this warning? I think it is perfectly 
reasonable to have a schema where multiple branches could be used as the 
default, and the errata makes it clear that the first one will be used.
   
   It might also mean we could change the above `filter` to a `find` to avoid 
some allocations.



##########
daffodil-test/src/test/resources/org/apache/daffodil/section15/choice_groups/ChoiceBranches.tdml:
##########
@@ -0,0 +1,278 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  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.
+-->
+
+<tdml:testSuite suiteName="choiceBranches" description="Tests for choice 
branch selection"
+  xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData"; 
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+  xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"; 
xmlns:xs="http://www.w3.org/2001/XMLSchema";
+  xmlns:ex="http://example.com";
+  xmlns:fn="http://www.w3.org/2005/xpath-functions";
+  xmlns="http://example.com";>
+
+  <tdml:defineSchema name="choiceBranches">
+    <xs:include 
schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+    <dfdl:format ref="ex:GeneralFormat"/>
+
+    <xs:group name="group_a">
+      <xs:sequence>
+        <xs:element name="a" type="xs:string" minOccurs="0" 
dfdl:lengthKind="explicit" dfdl:length="1"/>
+      </xs:sequence>
+    </xs:group>
+
+    <xs:group name="group_b">
+      <xs:sequence>
+        <xs:element name="b" type="xs:string" dfdl:lengthKind="explicit" 
dfdl:length="1" dfdl:outputValueCalc="{'b'}"/>
+      </xs:sequence>
+    </xs:group>
+
+    <xs:group name="optElement">
+      <xs:sequence>
+        <xs:element name="opt" type="xs:string" minOccurs="0" 
dfdl:lengthKind="explicit" dfdl:length="1"/>
+      </xs:sequence>
+    </xs:group>
+
+    <xs:group name="reqElements">
+      <xs:sequence>
+        <xs:element name="reqElements_a" type="xs:string" 
dfdl:lengthKind="explicit" dfdl:outputValueCalc="{'a'}"
+          dfdl:length="1"/>
+        <xs:element name="reqElements_b" type="xs:string" 
dfdl:lengthKind="explicit" dfdl:outputValueCalc="{'b'}"
+          dfdl:length="1"/>
+      </xs:sequence>
+    </xs:group>
+
+    <xs:element name="e1" dfdl:lengthKind="implicit">
+      <xs:complexType>
+        <xs:sequence>
+          <xs:choice>
+            <xs:group dfdl:initiator="first_defaultable" ref="ex:optElement"/> 
 <!-- canUnparseIfHidden/isOpen -->
+            <xs:sequence dfdl:initiator="second_open"> 
<!--isOpen/canUnparseIfHidden-->
+              <xs:sequence dfdl:hiddenGroupRef="ex:reqElements"/>
+              <xs:element name="char_a" minOccurs="0" type="xs:string" 
dfdl:lengthKind="explicit"
+                dfdl:length="1"/>
+            </xs:sequence>
+            <xs:sequence dfdl:initiator="third_empty"> <!-- isOpen/isEmpty -->
+              <xs:choice>
+                <xs:sequence dfdl:hiddenGroupRef="ex:group_a"/>
+                <xs:sequence dfdl:hiddenGroupRef="ex:group_b"/>
+              </xs:choice>
+            </xs:sequence>
+            <xs:element name="req" type="xs:string" dfdl:lengthKind="explicit" 
dfdl:length="1"/>
+          </xs:choice>
+          <xs:element name="after" type="xs:string" dfdl:lengthKind="explicit" 
dfdl:length="1"/>
+        </xs:sequence>
+      </xs:complexType>
+    </xs:element>
+
+    <xs:element name="e2" dfdl:lengthKind="implicit">
+      <xs:complexType>
+        <xs:sequence>
+          <xs:choice>
+            <xs:sequence dfdl:initiator="first_open"> 
<!--isOpen/canUnparseIfHidden-->
+              <xs:sequence dfdl:hiddenGroupRef="ex:reqElements"/>
+              <xs:element name="char_a" minOccurs="0" type="xs:string" 
dfdl:lengthKind="explicit"
+                dfdl:length="1"/>
+            </xs:sequence>
+            <xs:group dfdl:initiator="second_defaultable" 
ref="ex:optElement"/>  <!-- canUnparseIfHidden/isOpen -->
+            <xs:sequence dfdl:initiator="third_empty"> <!-- isOpen/isEmpty -->
+              <xs:choice>
+                <xs:sequence dfdl:hiddenGroupRef="ex:group_a"/>
+                <xs:sequence dfdl:hiddenGroupRef="ex:group_b"/>
+              </xs:choice>
+            </xs:sequence>
+            <xs:element name="req" type="xs:string" dfdl:lengthKind="explicit" 
dfdl:length="1"/>
+          </xs:choice>
+          <xs:element name="after" type="xs:string" dfdl:lengthKind="explicit" 
dfdl:length="1"/>
+        </xs:sequence>
+      </xs:complexType>
+    </xs:element>
+
+    <xs:element name="e3" dfdl:lengthKind="implicit">
+      <xs:complexType>
+        <xs:sequence>
+          <xs:choice>
+             <xs:sequence dfdl:initiator="first_empty"> <!-- isOpen/isEmpty -->
+               <xs:choice>
+                <xs:sequence dfdl:hiddenGroupRef="ex:group_a"/>
+                <xs:sequence dfdl:hiddenGroupRef="ex:group_b"/>
+              </xs:choice>
+            </xs:sequence>
+            <xs:sequence dfdl:initiator="second_open"> 
<!--isOpen/canUnparseIfHidden-->
+              <xs:sequence dfdl:hiddenGroupRef="ex:reqElements"/>
+              <xs:element name="char_a" minOccurs="0" type="xs:string" 
dfdl:lengthKind="explicit"
+                dfdl:length="1"/>
+            </xs:sequence>
+            <xs:group dfdl:initiator="third_defaultable" ref="ex:optElement"/> 
 <!-- canUnparseIfHidden/isOpen -->
+
+            <xs:element name="req" type="xs:string" dfdl:lengthKind="explicit" 
dfdl:length="1"/>
+          </xs:choice>
+          <xs:element name="after" type="xs:string" dfdl:lengthKind="explicit" 
dfdl:length="1"/>
+        </xs:sequence>
+      </xs:complexType>
+    </xs:element>
+
+  </tdml:defineSchema>
+
+  <tdml:unparserTestCase name="choiceBranch_e1" model="choiceBranches" 
root="e1">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+          <ex:e1>
+            <after>1</after>
+          </ex:e1>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>first_defaultable1</tdml:document>
+  </tdml:unparserTestCase>
+
+  <tdml:unparserTestCase name="choiceBranch_e2" model="choiceBranches" 
root="e2">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <e2>
+          <after>1</after>
+        </e2>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>first_openab1</tdml:document>
+  </tdml:unparserTestCase>
+
+  <tdml:unparserTestCase name="choiceBranch_e3" model="choiceBranches" 
root="e3">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <e3>
+          <after>1</after>
+        </e3>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>first_empty1</tdml:document>
+  </tdml:unparserTestCase>
+
+  <tdml:unparserTestCase name="choiceBranch_e1_req" model="choiceBranches" 
root="e1">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+          <ex:e1>
+            <req>0</req>
+            <after>1</after>
+          </ex:e1>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>01</tdml:document>
+  </tdml:unparserTestCase>
+
+  <tdml:unparserTestCase name="choiceBranch_e2_req" model="choiceBranches" 
root="e2">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <e2>
+          <req>0</req>
+          <after>1</after>
+        </e2>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>01</tdml:document>
+  </tdml:unparserTestCase>
+
+  <tdml:unparserTestCase name="choiceBranch_e3_req" model="choiceBranches" 
root="e3">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <e3>
+          <req>0</req>
+          <after>1</after>
+        </e3>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>01</tdml:document>
+  </tdml:unparserTestCase>
+
+  <tdml:unparserTestCase name="choiceBranch_e1_opt" model="choiceBranches" 
root="e1">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+          <ex:e1>
+            <opt>0</opt>
+            <after>1</after>
+          </ex:e1>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>first_defaultable01</tdml:document>
+  </tdml:unparserTestCase>
+
+  <tdml:unparserTestCase name="choiceBranch_e2_opt" model="choiceBranches" 
root="e2">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <e2>
+          <opt>0</opt>
+          <after>1</after>
+        </e2>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>second_defaultable01</tdml:document>
+  </tdml:unparserTestCase>
+
+  <tdml:unparserTestCase name="choiceBranch_e3_opt" model="choiceBranches" 
root="e3">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <e3>
+          <opt>0</opt>
+          <after>1</after>
+        </e3>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>third_defaultable01</tdml:document>
+
+    <tdml:warnings>
+      <tdml:warning>MultipleChoiceBranches</tdml:warning>
+      <tdml:warning>selecting</tdml:warning>
+      <tdml:warning>first branch</tdml:warning>
+      <tdml:warning>for unparsing</tdml:warning>
+    </tdml:warnings>
+  </tdml:unparserTestCase>
+
+
+  <tdml:unparserTestCase name="choiceBranch_e1_reqElements" 
model="choiceBranches" root="e1">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+          <ex:e1>
+            <char_a>c</char_a>
+            <after>1</after>
+          </ex:e1>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>second_openabc1</tdml:document>
+  </tdml:unparserTestCase>
+
+  <tdml:unparserTestCase name="choiceBranch_e2_reqElements" 
model="choiceBranches" root="e2">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <e2>
+          <char_a>c</char_a>
+          <after>1</after>
+        </e2>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>first_openabc1</tdml:document>
+  </tdml:unparserTestCase>
+
+  <tdml:unparserTestCase name="choiceBranch_e3_reqElements" 
model="choiceBranches" root="e3">
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <e3>
+          <char_a>c</char_a>
+          <after>1</after>
+        </e3>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+    <tdml:document>second_openabc1</tdml:document>
+  </tdml:unparserTestCase>
+

Review Comment:
   Nice tests!



##########
daffodil-test/src/test/scala/org/apache/daffodil/section15/choice_groups/TestChoiceBranches.scala:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.daffodil.section15.choice_groups
+
+import org.apache.daffodil.junit.tdml.TdmlSuite
+import org.apache.daffodil.junit.tdml.TdmlTests
+import org.apache.daffodil.tdml.Runner
+
+import org.junit.Test
+
+object TestChoiceBranches extends TdmlSuite {
+  val tdmlResource =
+    "/org/apache/daffodil/section15/choice_groups/ChoiceBranches.tdml"
+
+  override def createRunner() =
+    Runner(tdmlDir, tdmlFile, validateTDMLFile = false)

Review Comment:
   Can we remove this custom `createRunner`? The only difference between this 
and the default `createRunner` is this sets `validateTDMLFile = false`, but I 
*think* TDML file looks valid maybe isn't needed?



##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -80,35 +80,25 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
     // element event to be in the infoset (for round-tripping from parse). 
It's value will
     // get recomputed, but it can appear with a stale value (left-over from 
parse)
     // in the arriving infoset for unparse.
-    //
-    val optDefaultBranch = {
-      val optEmpty: Option[Term] = {
-        val emptyBranches = groupMembers.filter { gm =>
-          val ies = gm.identifyingEventsForChoiceBranch
-          ies.pnes.isEmpty // empty event list makes it the default, not 
simply isOpen
-        }
-        if (emptyBranches.length > 1)
-          SDW(
-            WarnID.MultipleChoiceBranches,
-            "Multiple choice branches with no required elements detected and 
the infoset does not specify a branch, selecting the first branch for unparsing"
-          )
-        emptyBranches.headOption
-      }
-      val optOpen: Option[Term] =
-        optEmpty.orElse {
-          groupMembers.find { gm =>
-            val ies = gm.identifyingEventsForChoiceBranch
-            ies.isOpen // first open one is used if there is no branch that 
has empty event list. (test ptg_1u)
-          }
-        }
-      val optDefault: Option[Term] =
-        optOpen.orElse {
-          groupMembers.find {
-            _.canUnparseIfHidden
-          } // optional, defaultable, OVC, or IVC
-        }
-      optDefault
+    // Rather than defaulting to the first empty branch, we instead default to 
the first
+    // empty, open or defaultable branch.
+    val emptyOpenOrDefault = groupMembers.filter {
+      case gm if gm.identifyingEventsForChoiceBranch.pnes.isEmpty => true
+      case gm if gm.identifyingEventsForChoiceBranch.isOpen => true
+      case gm if gm.canUnparseIfHidden => true
+      case _ => false
+    }
+    if (
+      emptyOpenOrDefault.length > 1 && emptyOpenOrDefault.forall(
+        _.identifyingEventsForChoiceBranch.pnes.isEmpty
+      )
+    ) {
+      SDW(
+        WarnID.MultipleChoiceBranches,
+        "Multiple choice branches with no required elements detected and the 
infoset does not specify a branch, selecting the first branch for unparsing"
+      )
     }
+    val optDefaultBranch = emptyOpenOrDefault.headOption

Review Comment:
   Since this is an opt, it means it is possible that we didn't find a default 
branch. In most cases this usually means that none of the the branches are 
empty/open/etc, and so we'll never need a default branch. But, could it also 
mean that there *are* empty branches, but they just can't be unparsed for some 
reason (e.g. `canUnparseIfHidden == false`)? In those cases would it mean that 
not having a default branch should be considered an error? We don't catch that 
here, but do we catch it somewhere else? Do we have any tests that error if we 
don't find a default branch but we might need one?



##########
daffodil-test/src/test/scala/org/apache/daffodil/section15/choice_groups/TestChoiceBranches.scala:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.daffodil.section15.choice_groups
+
+import org.apache.daffodil.junit.tdml.TdmlSuite
+import org.apache.daffodil.junit.tdml.TdmlTests
+import org.apache.daffodil.tdml.Runner
+
+import org.junit.Test
+
+object TestChoiceBranches extends TdmlSuite {
+  val tdmlResource =
+    "/org/apache/daffodil/section15/choice_groups/ChoiceBranches.tdml"
+
+  override def createRunner() =
+    Runner(tdmlDir, tdmlFile, validateTDMLFile = false)
+}
+
+class TestChoiceBranches extends TdmlTests {
+
+  val tdmlSuite = TestChoiceBranches
+
+  @Test def choiceBranch_e1(): Unit = test
+  @Test def choiceBranch_e2(): Unit = test
+  @Test def choiceBranch_e3(): Unit = test
+  @Test def choiceBranch_e1_req(): Unit = test
+  @Test def choiceBranch_e2_req(): Unit = test
+  @Test def choiceBranch_e3_req(): Unit = test
+  @Test def choiceBranch_e1_opt(): Unit = test
+  @Test def choiceBranch_e2_opt(): Unit = test
+  @Test def choiceBranch_e3_opt(): Unit = test
+  @Test def choiceBranch_e1_reqElements(): Unit = test
+  @Test def choiceBranch_e2_reqElements(): Unit = test
+  @Test def choiceBranch_e3_reqElements(): Unit = test

Review Comment:
   The convention we have been using for these tests is to exclude the 
parentheses  and the Unit return type, e.g.
   
   ```scala
   @Test def choiceBranch_e3_reqElements = test
   ```
   
   They aren't required and keeps our tests a bit more minimal.



-- 
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]

Reply via email to