You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2020/05/19 15:10:09 UTC
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #384: Validate PadCharacter with pattern facet
mbeckerle commented on a change in pull request #384:
URL: https://github.com/apache/incubator-daffodil/pull/384#discussion_r427360685
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/DFDL_part1_simpletypes.xsd
##########
@@ -17,9 +17,9 @@
-->
<xsd:schema targetNamespace="http://www.ogf.org/dfdl/dfdl-1.0/"
- xmlns:xsd="http://www.w3.org/2001/XMLSchema"
- xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
- attributeFormDefault="unqualified" elementFormDefault="qualified">
Review comment:
We tolerate attributeFormDefault on purpose even though it is unused by DFDL. This was because some IBM tooling put it into DFDL schemas. So long as there is some other test that shows we're tolerating it elsewhere I'm fine with removing it from here.
##########
File path: daffodil-test-ibm1/src/test/resources/test-suite/tresys-contributed/AT.dfdl.xsd
##########
@@ -116,8 +116,10 @@
<!-- this have BEFORE timing, so for the first local_name
$tns:counter = 1 -->
<!-- NOTE: THIS REQUIRES dfdl:newVariableInstance BECAUSE VARIABLES ARE SINGLE ASSIGNMENT IN DFDL -->
- <!-- Updating test to use newVariableInstance but the feature is not yet implemented
- so this will need to be reviewed -Beth 8/29/13 -->
+ <!-- Using newVariableInstance here gives SDE:
+ newVariableInstance may only be used on group
+ reference, sequence or choice so this will
+ need to be reviewed (5/18/20) -->
<dfdl:newVariableInstance ref="tns:counter" defaultValue="{ $tns:counter + 1 }" />
Review comment:
This whole test and example can only be a negative test if it's trying to use a variable as an accumulator, and trying to use newVariableInstance on an element.
These tests with the 2-letter names like AT are really old (like 10 years now), and concepts in DFDL shifted out from under them.
I bet we can just drop test AT without losing any coverage.
##########
File path: daffodil-tdml-processor/src/test/scala/org/apache/daffodil/tdml/TestTDMLRunner2.scala
##########
@@ -424,11 +424,26 @@ abc # a comment
}
@Test def testTDMLWithInvalidDFDLSchemaEmbedded() = {
+ val testSuite =
+ <tdml:testSuite suiteName="theSuiteName" xmlns:tns={ tns } xmlns:tdml={ tdml } xmlns:dfdl={ dfdl } xmlns:xs={ xsd }>
+ <!-- This embedded schema has validation errors. -->
+ <tdml:defineSchema name="mySchema">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <xs:format ref="tns:GeneralFormat" />
+ <xs:element name="data" type="fn:string" dfdl:lengthKind="delimited" />
+ </tdml:defineSchema>
+ <tdml:parserTestCase name="test1" root="data" model="mySchema">
+ <tdml:document>
+ <tdml:documentPart type="text"><![CDATA[abcdef]]></tdml:documentPart>
+ </tdml:document>
+ <tdml:errors>
+ <tdml:error>Cannot resolve 'fn:string' as a QName</tdml:error>
+ </tdml:errors>
+ </tdml:parserTestCase>
+ </tdml:testSuite>
- val testDir = ""
- val aa = testDir + "tdml-with-embedded-schema-errors.tdml"
- lazy val runner = new DFDLTestSuite(Misc.getRequiredResource(aa), validateTDMLFile = false, validateDFDLSchemas = true)
- runner.runOneTest("test1")
+ lazy val ts = new DFDLTestSuite(testSuite, validateTDMLFile = false, validateDFDLSchemas = true)
Review comment:
We have class Runner now with a factory object Runner. This is preferred over using DFDLTestSuite directly. We never cut everything over to Runner, hoping to do that incrementally. Eventually we'd like to make DFDLTestSuite package private so it isn't usable.
##########
File path: daffodil-tdml-processor/src/test/scala/org/apache/daffodil/tdml/TestTDMLRunner2.scala
##########
@@ -424,11 +424,26 @@ abc # a comment
}
@Test def testTDMLWithInvalidDFDLSchemaEmbedded() = {
+ val testSuite =
+ <tdml:testSuite suiteName="theSuiteName" xmlns:tns={ tns } xmlns:tdml={ tdml } xmlns:dfdl={ dfdl } xmlns:xs={ xsd }>
+ <!-- This embedded schema has validation errors. -->
+ <tdml:defineSchema name="mySchema">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <xs:format ref="tns:GeneralFormat" />
+ <xs:element name="data" type="fn:string" dfdl:lengthKind="delimited" />
+ </tdml:defineSchema>
+ <tdml:parserTestCase name="test1" root="data" model="mySchema">
+ <tdml:document>
+ <tdml:documentPart type="text"><![CDATA[abcdef]]></tdml:documentPart>
+ </tdml:document>
+ <tdml:errors>
+ <tdml:error>Cannot resolve 'fn:string' as a QName</tdml:error>
Review comment:
The reason these tdml:error strings are generally so sparse is so that the tests aren't so fragile if an error message evolves slightly. E.g., this test should pass so long as the error message mentions fn:string, and mentions "resolve". Beyond that, if it says "Cannot" or "QName", or the wording changes to use double quotes around "fn:string" instead of single quotes, or changes to say "A QName is required." instead of "as a QName", we don't really care.
I would suggest this should be <tdml:error>resolve</tdml:error><tdml:error>fn:string</tdml:error> so that it is just looking for those two concepts.
##########
File path: daffodil-test-ibm1/src/test/scala/org/apache/daffodil/TresysTests.scala
##########
@@ -175,6 +175,7 @@ class TresysTests {
// @Test def test_AP000() { runnerAP.runOneTest("AP000") } // lengthKind endOfParent - DAFFODIL-567
+ // Were these tests overlooked while fixing DAFFODIL-341?
Review comment:
Yes I believe you are correct. If we have coverage elsewhere of catching multiple-assignment and I know we have tests of newVariableInstance elsewhere. Mostlikely we should just drop these tests.
##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section13/validation_errors/PadCharacter.scala
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.section13.validation_errors
+
+import org.apache.daffodil.tdml.DFDLTestSuite
+import org.apache.daffodil.xml.XMLUtils
+import org.junit.Test
+
+class PadCharacter {
+
+ val tdml = XMLUtils.TDML_NAMESPACE
+ val dfdl = XMLUtils.DFDL_NAMESPACE
+ val xsd = XMLUtils.XSD_NAMESPACE
+ val tns = XMLUtils.EXAMPLE_NAMESPACE
+
+ @Test def test_short_form_pad_char() = {
+ // This test demonstrates that you cannot use a literal whitespace character
+ // in the short form of the pad character's property binding syntax
+ val testSuite =
+ <tdml:testSuite suiteName="PadCharTests" description="Section 13 - Pad Character Tests"
+ xmlns:tdml={ tdml } xmlns:dfdl={ dfdl } xmlns:xs={ xsd } xmlns:tns={ tns }>
+ <tdml:defineSchema name="padCharSchema">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd" />
+ <dfdl:format ref="tns:GeneralFormat" textTrimKind="padChar" textPadKind="padChar" />
+ <xs:element name="shortForm" type="xs:string" dfdl:textStringPadCharacter=" " />
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="short_form_pad_char" root="shortForm" model="padCharSchema">
+ <tdml:document>
+ <tdml:documentPart type="text"><![CDATA[ hello]]></tdml:documentPart>
+ </tdml:document>
+ <tdml:errors>
+ <tdml:error>Value ' ' is not facet-valid</tdml:error>
Review comment:
I would make this <tdml:error>facet</tdml:error><tdml:error>valid</tdml:error><tdml:error>NonEmptyStringLiteral</tdml:error> so that it's not sensitive to wording or quoting conventions.
Same idea applies to your other 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:
users@infra.apache.org