Cross Testing Capability with IBM DFDL #129
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
!!! NOTE: There are still some tests failing because of TDML Runner changes with how the root element is determined, and being more strict about the root element. Nevertheless this is a useful point for a code review. !!! TDML Runner now implements everything it does in terms of an abstract DFDL processor defined in the tdml.processor package. Yes, this is yet another API for Daffodil, but it is narrower and not as comprehensive as the SAPI/JAPI stuff. It's only and exactly what the TDML runner uses to drive tests. Even Daffodil's native testing is done by way of a Daffodil version of this abstract processor. The abstract processors are always dynamically loaded from the classpath (even Daffodil's own - just to insure we're always testing this mechanism.) Changed URISchemaSource from case class to regular class so we can derive from it. Enable TDMLDFDLProcessors to force skip of left-over-data checking. Added explicit xs:include of DFDLGeneralFormat.dfdl.xsd to all TDML. DAFFODIL-723
DAFFODIL-2014
mbeckerle
commented
Oct 26, 2018
daffodil-core/src/main/scala/org/apache/daffodil/compiler/Compiler.scala
Outdated
Show resolved
Hide resolved
mbeckerle
commented
Oct 26, 2018
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/DFDL_part2_attributes.xsd
Show resolved
Hide resolved
|
+1 with removal or changes of commented out code |
| val duplicateTestCases = testCases.groupBy { | ||
| _.tcName | ||
| }.filter { case (name, seq) => seq.length > 1 } | ||
| if (duplicateTestCases.nonEmpty) { | ||
| duplicateTestCases.foreach { | ||
| case (name, _) => | ||
| System.err.println("TDML Runner: More than one test case for name '%s'.".format(name)) |
There was a problem hiding this comment.
Should duplicate test names be a TDMLException?
There was a problem hiding this comment.
It is tolerated because we didn't want to hunt them down and fix them. They were there in the test suite, quite a few, at one time. I haven't checked lately but there's a JIRA ticket to fix them I think, at which point yes, this should be an exception.
daffodil-tdml/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
Outdated
Show resolved
Hide resolved
These were wrong because the comparison logic in the test rig was tolerating them being incorrect, but as part of making the TDML runner into a viable cross-testing rig, these things are being tightened up. The root element named by a test must match that of the infoset element. (in fact the root element declaration is unnecessary, as the root will be taken from the infoset's root element. Moved all the IBM contributed tests to the daffodil-test-ibm1 module out of the daffodil-test module. There were redundant tests being run in both places. Probably this was due to an ambitious QA effort to reorganize the IBM-supplied tests to determine requirements coverage on the DFDL Specification, along with the Daffodil-project tests... I mean why have more than one test for a given requirement. However, this was not done right - moving the tests from one place to another - instead tests were left in both places. So I have moved all the tests that use the files supplied orignally by IBM back to the daffodil-test-ibm1 module. So there is no longer any ibm-tests subdir of the daffodil-test module. (Reminder: IBM supplied these a long time ago, and they are fully approved for inclusion into Daffodil, with ASF licensing, etc. There is no issue of source code origins here. ) DAFFODIL-723
This property is deprecated in favor of textStandardExponentRep (because it can be more than one character). However the old name is still used by some schemas used with IBM DFDL, so for cross testing it is good to have this also supported by daffodil. DAFFODIL-2015
Now only requests this property if all the criteria are met. DAFFODIL-1616
Log an Info for tests that run with IBM DFDL. Revised try/catch logic around finding the processor dynamically in TDML runner.
| } else 0 | ||
| res | ||
| } | ||
|
|
There was a problem hiding this comment.
This change seems unrelated to this pull request. Can it be pulled out into a separate PR with a separate bug? Would be easier to review just the textOutputMinLength change.
There was a problem hiding this comment.
I'll look into this. Might be hard to tease apart such that these can be isolated.
| @@ -685,7 +720,7 @@ trait SimpleTypeRuntimeValuedPropertiesMixin | |||
|
|
|||
| private lazy val textStandardExponentRepExpr = LV('textStandardExponentRep) { | |||
| val qn = this.qNameForProperty("textStandardExponentRep") | |||
| val c = ExpressionCompilers.String.compileProperty(qn, NodeInfo.String, textStandardExponentRepRaw, decl) | |||
| val c = ExpressionCompilers.String.compileProperty(qn, NodeInfo.String, textStandardExponentRep, decl) | |||
There was a problem hiding this comment.
Same here, seems unrelated to the TDML runner changes. Can it be pulled out into a separate PR?
daffodil-core/src/main/scala/org/apache/daffodil/dsom/TermEncodingMixin.scala
Show resolved
Hide resolved
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/DFDL_part2_attributes.xsd
Show resolved
Hide resolved
daffodil-tdml/src/main/scala/org/apache/daffodil/tdml/RunnerFactory.scala
Show resolved
Hide resolved
|
I have incorporated all the feedback from this code review into the code base. However, the change set is quite large, and quite a bit more refactoring was done. So it doesn't really make sense to continue this review by adding more changes to this. I'm going to start a new PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This review goes with one that is outside of the Daffodil repo here:
OpenDFDL/ibmDFDLCrossTester#1
That's the "plug in", and the changes to Daffodil will find that on the classpath and use it if it is found and tests are requesting it.
When reviewing - most of the action is in the TDML Runner. Changes elsewhere are sweeping small things that had to be uniformly fixed, (like every defineSchema in a TDML now must explicitly include any includes, and textBiDi was wrong case on the "D".
Vast bulk of the important changes are TDMLRunner enhancements. Specifically, there is now this tdml.processor package. In it is the Daffodil TDML Processor, and a trait processors must implement. The IBM flavor of this is in a separate git repo on github, and isn't part of daffodil see PR link above.
DAFFODIL-723