stevedlawrence commented on a change in pull request #394: URL: https://github.com/apache/incubator-daffodil/pull/394#discussion_r455801271
########## File path: daffodil-core/src/test/scala/org/apache/daffodil/dsom/walker/TestDSOMWalker.scala ########## @@ -0,0 +1,229 @@ +/* + * 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.dsom.walker + +import org.apache.daffodil.util._ +import org.apache.daffodil.compiler.{ Compiler, ProcessorFactory } +import org.junit.Test +import org.junit.Assert._ + +class TestDSOMWalker { + + @Test def testComplexTypes(): Unit = { + val testSchema = SchemaUtils.dfdlTestSchema( + <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>, + <dfdl:format ref="ex:GeneralFormat" + alignment="implicit" alignmentUnits="bits" occursCountKind="implicit" + lengthKind="delimited" encoding="ASCII"/>, + <xs:element name="PersonData"> + <xs:complexType> + <xs:sequence> + <xs:choice> + <xs:element name="age" type="xs:int" minOccurs="1" maxOccurs="1"/> + </xs:choice> + <xs:group ref="testGroup" /> + </xs:sequence> + </xs:complexType> + </xs:element> + <xs:group name="testGroup"> + <xs:sequence /> + </xs:group> + ) + val pf: ProcessorFactory = Compiler().compileNode(testSchema) + assertEquals(s"This basic Schema: $testSchema should compile; here are some diagnostics: ${pf.getDiagnostics}", false, pf.isError) + val walker: BasicWalker = new BasicWalker() + walker.walkFromRoot(pf.rootView) + val nodeStack: List[Either[TermView, TypeView]] = walker.nodeArr.toList + assertEquals(s"Node Stack $nodeStack did not have the expected number of elements", 7, nodeStack.size) + nodeStack.head match { + case Left(personElement: RootView) => + assertEquals("The root element should be named 'PersonData'", "PersonData", personElement.name) + case Left(someClass) => fail(s"The Root element was not of type RootView, it was a ${someClass.getClass}!") + case Right(someClass) => fail(s"The Root element was not of type RootView, it was a ${someClass.getClass}!") + } + nodeStack(1) match { + case Right(_: ComplexTypeView) => + case Left(someClass) => fail(s"The Root element did not contain a complexType wrapper child, it was a ${someClass.getClass}!") + case Right(someClass) => fail(s"The Root element element did not contain a complexType wrapper child, it was a ${someClass.getClass}!") + } + nodeStack(2) match { + case Left(_: SequenceView) => + case Left(someClass) => fail(s"The complexType element did not contain a Sequence child, it was a ${someClass.getClass}!") + case Right(someClass) => fail(s"The complexType element did not contain a Sequence child, it was a ${someClass.getClass}!") + } + nodeStack(3) match { + case Left(_: ChoiceView) => + case Left(someClass) => fail(s"The Sequence element did not contain a Choice child, it was a ${someClass.getClass}!") + case Right(someClass) => fail(s"The Sequence element did not contain a Choice child, it was a ${someClass.getClass}!") + } + nodeStack(4) match { + case Left(nameElement: ElementBaseView) => + assertEquals("The second child element should be named 'age'", "age", nameElement.name) + assertEquals("The second child element should be a simple type", true, nameElement.isSimpleType) + case Left(someClass) => fail(s"The Sequence element did not contain a second Element child, it was a ${someClass.getClass}!") + case Right(someClass) => fail(s"The Sequence element did not contain a second Element child, it was a ${someClass.getClass}!") + } + nodeStack(5) match { + case Right(simpleType: SimpleTypeView) => + simpleType.primType match { + case _:IntView => + case _ => fail(s"The 'age' element should be of simple type Int") + } + case Left(someClass) => fail(s"The 'age' element did not have a SimpleTypeView, it was a ${someClass.getClass}!") + case Right(someClass) => fail(s"The 'age' element did not have a SimpleTypeView, it was a ${someClass.getClass}!") + } + nodeStack(6) match { + case Left(_: GroupRefView) => + case Left(someClass) => fail(s"The Sequence element did not contain a GroupRef child, it was a ${someClass.getClass}!") + case Right(someClass) => fail(s"The Sequence element did not contain a GroupRef child, it was a ${someClass.getClass}!") + } + } Review comment: These feels a little verbose to me. All the fail messages are perhaps helpful when a test fails, but that should be rare, and it makes it difficult to visually determine that the tests are checking the right things, which I'd argue is more important. I think part of the problem this looks so verbose is the the node stack being an Either, so you need to do a bunch of left/right checks. But that doesn't give us much. You still need to do type matcing inside the Left/Right, so the Left/Right just adds a layer of indirection. Instead, you can just make nodeArray be an Array[AnyRef]. Then, if we removed all the matching and messages, the above can be condensed down to something like this: ```scala assertEquals(7, nodeStack.size) assertTrue(nodeStack(0).isInstanceOf[RootView]) assertEquals(nodeStack(0).asInstanceOf[RootView].name, "PersonData") assertTrue(nodeStack(1).isInstanceOf[ComplexTypeView]) assertTrue(nodeStack(2).isInstanceOf[SequenceView]) assertTrue(nodeStack(3).isInstanceOf[ChoiceView]) assertTrue(nodeStack(4).isInstanceOf[ElementBaseView]) assertEquals(nodeStack(4).asInstanceOf[ElementBaseView].name, "age") assertEquals(nodeStack(4).asInstanceOf[ElementBaseView].isSimpleType, true) assertTrue(nodeStack(5).isInstanceOf[SimpleTypeView]) assertTrue(nodeStack(5).asInstanceOf[SimpleTypeView].primType.isInstanceOf[IntView]) assertTrue(nodeStack(6).isInstanceOf[GroupRefView]) ``` Which I think is much easier to visually confirm that we're checking the right things. The suggestion I previously made about having concrete implemenations of the Views and an equal function make these checks a bit more automated and maybe a bit more clear (probably gets rid of all the instance of stuff), at the expense of needing a bit more code. But something more along the lines of this doesn't seem unreasonable to me. Note that this is much less scala-y, but readability often trumps that, especially when it comes to tests. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
