stevedlawrence commented on code in PR #1204: URL: https://github.com/apache/daffodil/pull/1204#discussion_r1567483480
########## daffodil-tdml-processor/src/test/resources/test/tdml/testTDMLErrorsWarningsMatchAttribute.tdml: ########## @@ -0,0 +1,150 @@ +<?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="tdml errors/warnings match attribute" + description="Tests for TDML Runner errors/warnings match attribute any, all and none" + 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"; + defaultRoundTrip="false"> + + <tdml:defineSchema name="causesWarnings"> + <xs:include schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/> + <dfdl:format ref="ex:GeneralFormat" lengthKind="delimited" encoding="utf-8" representation="text"/> + <xs:element name="elem" type="xs:string"> + <xs:annotation> + <xs:appinfo source="http://www.ogf.org/dfdl/dfdl-1.0/"; /> + </xs:annotation> + </xs:element> + </tdml:defineSchema> + + <tdml:parserTestCase name="getsWarningExpectsNoErrors" root="elem" + model="causesWarnings" + description=""> + <tdml:document><![CDATA[test]]></tdml:document> + + <tdml:infoset> + <tdml:dfdlInfoset> + <elem>test</elem> + </tdml:dfdlInfoset> + </tdml:infoset> + + <tdml:errors match="none"> + <tdml:error>Schema Definition Error</tdml:error> + </tdml:errors> + + <tdml:warnings match="any"> + <tdml:warning>THIS WILL NOT MATCH</tdml:warning> + <tdml:warning>Schema Definition Warning</tdml:warning> + <tdml:warning>The xs:appinfo source attribute value 'http://www.ogf.org/dfdl/dfdl-1.0/' should be 'http://www.ogf.org/dfdl/'.</tdml:warning> + </tdml:warnings> + + <tdml:validationErrors match="none"> + <tdml:error>pattern facet</tdml:error> + <tdml:error>WILL NOT MATCH ON ANY</tdml:error> + </tdml:validationErrors> + + </tdml:parserTestCase> + +<tdml:defineSchema name="causesErrors" elementFormDefault="unqualified"> + + <xs:include schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/> + <dfdl:format ref="ex:GeneralFormat" lengthKind="explicit" occursCountKind="implicit"/> + + <xs:element name="causesUnparseError" dfdl:lengthKind="implicit"> + <xs:complexType> + <xs:sequence> + <xs:element name="A" type="xs:string" dfdl:length="1" /> + <xs:element name="AmbigElt" type="xs:string" dfdl:length="1" /> + <xs:element name="B" type="xs:string" dfdl:length="1" /> + <xs:element name="AmbigElt" type="xs:string" dfdl:length="1" minOccurs="0" /> + <xs:element name="UnparseFails" type="xs:string" dfdl:length="1" + dfdl:outputValueCalc="{ ../AmbigElt }"/> + </xs:sequence> + </xs:complexType> + </xs:element> + + <xs:element name="causesValidationError" dfdl:lengthKind="implicit"> + <xs:complexType> + <xs:sequence> + <xs:element name="A" dfdl:length="1" dfdl:inputValueCalc="{ 'A' }"> + <xs:simpleType> + <xs:restriction base="xs:string"> + <xs:pattern value="[^A-FH-OQ-Z]"/> + </xs:restriction> + </xs:simpleType> + </xs:element> + </xs:sequence> + </xs:complexType> + </xs:element> + +</tdml:defineSchema> + +<tdml:unparserTestCase name="getUnparserWarningWhenExpectingError" + root="causesUnparseError" model="causesErrors"> + <tdml:infoset> + <tdml:dfdlInfoset> + <ex:causesUnparseError> + <A>1</A> + <AmbigElt>2</AmbigElt> + <B>3</B> + <AmbigElt>4</AmbigElt> + </ex:causesUnparseError> + </tdml:dfdlInfoset> + </tdml:infoset> + + <tdml:document><![CDATA[12344]]></tdml:document> + + <tdml:errors match="any"> + <tdml:error>WILL NOT MATCH ON ANY</tdml:error> + <tdml:error>Schema Definition Error</tdml:error> + </tdml:errors> + + <tdml:warnings match="any"> + <tdml:warning>WILL NOT MATCH ON ANY</tdml:warning> + <tdml:warning>Schema Definition Warning</tdml:warning> + <tdml:warning>AmbigElt</tdml:warning> + <tdml:warning>query-style</tdml:warning> + </tdml:warnings> +</tdml:unparserTestCase> + + +<tdml:parserTestCase name="expectsAnyValidationError" root="causesValidationError" + model="causesErrors" validation="on"> + + <tdml:document/> + + <tdml:infoset> + <tdml:dfdlInfoset> + <ex:causesValidationError> + <A>A</A> + </ex:causesValidationError> + </tdml:dfdlInfoset> + </tdml:infoset> + + <tdml:validationErrors match="any"> + <tdml:error>value 'A' of element 'A' is not valid</tdml:error> + <tdml:error>WILL NOT MATCH ON ANY</tdml:error> + </tdml:validationErrors> +</tdml:parserTestCase> Review Comment: Does this support a way to say that no diagnostics of a type should exist? Or maybe a way to say there must be warnings or errors, but that we don't necessarily care what they are? Maybe an empty tag with match="all" means an diag type must exist, but we don't care about the message. ```xml <tdml:errors match="all" /> ``` And a empty tag with match="none" means a diag type must not exist: ```xml <tdml:warnings match="none" /> ``` I'm not sure if that logic makes sense or if it should be reversed, but something along those lines might be useful. ########## daffodil-lib/src/main/resources/org/apache/daffodil/xsd/tdml.xsd: ########## @@ -349,6 +349,7 @@ <xs:restriction base="xs:string"> <xs:enumeration value="all"/> <xs:enumeration value="any"/> + <xs:enumeration value="none"/> Review Comment: I didn't even know this "match" attribute existed. I guess we never implemented it? I'm wonder if it would be useful to have something a big more complex? For example, I could imagine a test might want to do something like diags "a", "b", and "c", must all appear, one of "m" or "n" must apear, and none of "x", "y", and "z" must appear. Maybe we need to allow muptilpe <tdml:error> tags? Then this would be described like ```xml <parserTestCase ...> ... <errors match="all"> <error>a</error> <error>b</error> <error>c</error> </errors> <errors match="any"> <error>m</error> <error>n</error> </errors> <errors match="none"> <error>x</error> <error>y</error> <error>z</error> </errors> </parserTestCase> ``` Maybe that's unneeded complexity though? ########## daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala: ########## @@ -1844,12 +1944,99 @@ object VerifyTestCase { } /** - * Strip location info from schema context off diag string message - * @param msg diag string message - * @return diag message without location information + * Do diagnostics verification + * + * @param actualDiags Actual diagnostics produced + * @param expectedDiags Expected diagnostics from test cases + * @param implString Implementation string */ - def stripLocationInformation(msg: String): String = { - msg.replaceAll("Location line.*", "") + def verifyAnyOfDiagnosticsFound( + actualDiags: Seq[Diagnostic], + expectedDiags: Option[ErrorWarningBase], + implString: Option[String], + ) = { + + val actualDiagMsgs = actualDiags.map { + _.toString() + } + val expectedDiagMsgs = expectedDiags + .map { + _.messages + } + .getOrElse(Nil) + + if (expectedDiags.isDefined && actualDiags.isEmpty) { + throw TDMLException( + """"Diagnostic message(s) were expected but not found."""" + + "\n" + + """Expected: """ + expectedDiagMsgs.mkString("\n") + + (if (actualDiagMsgs.isEmpty) + "\n No diagnostic messages were issued." + else + "\n The actual diagnostics messages were: " + actualDiagMsgs.mkString("\n")), + implString, + ) + } + + // must find at least one expected warning message within some + // actual warning message. + val wasFound = expectedDiags.exists { expectedDiag => + expectedDiag.messages.exists { expectedMsg => + val wasFound = actualDiags.exists { actualDiag => + val actualDiagMsg = actualDiag.getMessage() + actualDiag.isError == expectedDiag.isError && + actualDiagMsg.toLowerCase.contains(expectedMsg.toLowerCase) + } + wasFound + } + } + if (!wasFound) { + throw TDMLException( + s"""Did not find any of the diagnostic ${expectedDiags.head.diagnosticType} message """" + + expectedDiagMsgs.mkString("\n") + + """" in any of the actual diagnostic messages: """ + "\n" + + actualDiagMsgs.mkString("\n"), + implString, + ) + } + } + + def verifyNoneOfDiagnosticsFound( + actualDiags: Seq[Diagnostic], + expectedDiags: Option[ErrorWarningBase], + implString: Option[String], + ): Unit = { + + val actualDiagMsgs = actualDiags.map { + _.toString() + } + val expectedDiagMsgs = expectedDiags + .map { + _.messages + } + .getOrElse(Nil) + + // must not find any of the expected warning message within some + // actual warning message. + val wasFound = expectedDiags.exists { expectedDiag => + expectedDiag.messages.exists { expectedMsg => + val wasFound = actualDiags.exists { actualDiag => + val actualDiagMsg = actualDiag.getMessage() + actualDiag.isError == expectedDiag.isError && + actualDiagMsg.toLowerCase.contains(expectedMsg.toLowerCase) + } + wasFound + } + } + if (wasFound) { + throw TDMLException( + s"""Found one of diagnostic ${expectedDiags.head.diagnosticType} message """" + + expectedDiagMsgs.mkString("\n") + + """" in some of the actual diagnostic messages: """ + "\n" + + actualDiagMsgs.mkString("\n"), + implString, + ) + } Review Comment: It might be useful to reduce duplication by combining these functions into a single one that accepts a mode. It feels like are the core they are pretty similar just with some minor differences. For example, you could maybe do something like: ```scala val diagsFound = expectedDiags.map { diag => val wasFound = actualDiags.contains(diag) (diag, wasFound) } matchAttr match { case "all" if (!diagsFound.forall(_._2)) => { throw TDMLException(...) } case "any" if (!diagsFound.exists(_._2)) => { throw TDMLException(...) } case "none" if (diagsFound.exists(_._2)) => { throw TDMLException(...) } } ``` So the logic to determine which diags are found is exactly the same with out copy/paste, and we create a Seq that determines which diags are found/not found. Then a match/case does the logic to figure out which of those were failures depending on the match mode and to create the appropriate exception. -- 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]
