tuxji commented on code in PR #800:
URL: https://github.com/apache/daffodil/pull/800#discussion_r891713141
##########
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd:
##########
@@ -587,15 +587,23 @@
<xs:restriction base="dfdlx:ParseUnparsePolicyEnum" />
</xs:simpleType>
<xs:simpleType>
- <xs:restriction base="xs:string">
+ <xs:restriction base="xs:token">
<xs:enumeration value="fromRoot" />
</xs:restriction>
</xs:simpleType>
</xs:union>
</xs:simpleType>
+ <xs:simpleType name="TunableTDMLImplementation">
Review Comment:
I removed "Tunable" from the name but the type was no longer generated
automatically. It turns out that TunableGenerator.scala filters simple types
to only these which start with "Tunable" so we have to keep the "Tunable".
##########
daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala:
##########
@@ -397,6 +407,10 @@ class CLIConf(arguments: Array[String]) extends
scallop.ScallopConf(arguments) {
descr("List or execute TDML tests")
helpWidth(width)
+ val implementation = opt[TDMLImplementation](short = 'I', argName =
"implementation",
+ descr = "Implementation to run TDML tests. Choose one of %s. Defaults to
%s."
+ .format(TDMLImplementation.allValues.mkString(", "),
TDMLImplementation.allValues.head.toString),
+ default = TDMLImplementation.optionStringToEnum("I",
sys.env.getOrElse("TDML_IMPLEMENTATION", "")))
Review Comment:
I will simplify Main.scala to pass an optTDMLImplementation to Runner (it
will be defined only if the CLI -I option is given on the command line,
otherwise it will be None). I will move the TDML_IMPLEMENTATION lookup to
TDMLRunner.scala so that all the ways that people can run tests from the CLI,
sbt, or the IDE can use that environment variable if they want.
I agree that `-I ibm` should do something different than `-I daffodil`; I
hope someone works on that after this pull request goes in.
##########
daffodil-tdml-processor/src/test/java/org/apache/daffodil/tdml/TestRunnerFactory.java:
##########
@@ -39,6 +40,7 @@ public void testPrimaryConstructor() {
Right<scala.xml.Elem, String> rightURI = new Right<>(tdmlUri.toString());
Runner runner = new Runner(
rightURI,
+ TDMLImplementation.allValues().head(),
Review Comment:
Since I will change the parameter's type as well, I will pass
Option.apply(null) instead of TDMLImplementation.allValues().head(). I noticed
IDEA warns that TestRunnerFactory is calling Runner's private constructor, but
sbt still compiles TestRunnerFactory fine, so that's why I think Java can call
a Scala private constructor (TestRunnerFactory is written in Java, not Scala).
##########
daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/ex_nums.tdml:
##########
@@ -38,21 +38,25 @@
<tdml:defineConfig name="config-runtime2">
<daf:tunables>
- <daf:tdmlImplementation>daffodil-runtime2</daf:tdmlImplementation>
+ <daf:tdmlImplementation>daffodilC</daf:tdmlImplementation>
</daf:tunables>
</tdml:defineConfig>
Review Comment:
Yes, I think we have a stable design point where the Scala junit test driver
code can run tests with various TDML implementations programmatically, you can
run TDML test files from the "daffodil test" command line with various TDML
implementations manually, and you can run tests from the "sbt test" command
line with various TDML implementations by setting the TDML_IMPLEMENTATION
environment variable. You just have to be aware that setting the
TDML_IMPLEMENTATION environment variable will not override tests like `sbt
"testOnly org.apache.daffodil.runtime2.TestExNums"` which already have TDML
implementations passed programmatically from their junit test driver code. But
most existing tests won't be having TDML implementations passed to them
programmatically, so setting TDML_IMPLEMENTATION will work on these tests and
you also will see error messages if you set TDML_IMPLEMENTATION to an invalid
implementation name.
```shell
$ export TDML_IMPLEMENTATION=not
$ sbt "testOnly org.apache.daffodil.tdml.TestRunnerFactory"
...
[error] Test
org.apache.daffodil.tdml.TestRunnerFactory.testTwoArgConstructor failed:
org.apache.daffodil.tdml.TDMLExceptionImpl: TDML_IMPLEMENTATION=not does not
give a valid TDML implementation, took 0.949 sec
...
```
##########
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd:
##########
@@ -502,11 +502,11 @@
</xs:documentation>
</xs:annotation>
</xs:element>
- <xs:element name="tdmlImplementation" type="xs:string"
default="daffodil" minOccurs="0">
+ <xs:element name="tdmlImplementation"
type="daf:TunableTDMLImplementation" default="daffodil" minOccurs="0">
Review Comment:
Yes, not being able to override a daffodilC tunable if you want to test with
runtime1 is overly complicated. I will throw out the tunable and do what you
suggest.
##########
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/unparsers.c:
##########
@@ -401,27 +401,24 @@ unparse_le_uint8(uint8_t number, size_t num_bits, UState
*ustate)
unparse_endian_uint64(LITTLE_ENDIAN_DATA, number, num_bits, ustate);
}
-// Add fill bytes until end bitPos0b is reached
+// Unparse fill bits until end bitPos0b is reached
void
-unparse_fill_bytes(size_t end_bitPos0b, const uint8_t fill_byte, UState
*ustate)
+unparse_fill_bits(size_t end_bitPos0b, const uint8_t fill_byte, UState *ustate)
{
- union
- {
- uint8_t bytes[1];
- } buffer;
- buffer.bytes[0] = fill_byte;
+ assert(ustate->bitPos0b <= end_bitPos0b);
- // Update our last successful write position only if this loop
- // finishes without any errors
- size_t current_bitPos0b = ustate->bitPos0b;
- while (current_bitPos0b < end_bitPos0b)
+ size_t fill_bits = end_bitPos0b - ustate->bitPos0b;
+ while (fill_bits)
{
- write_bits(buffer.bytes, BYTE_WIDTH, ustate);
+ size_t num_bits = (fill_bits >= BYTE_WIDTH) ? BYTE_WIDTH : fill_bits;
+ write_bits(&fill_byte, num_bits, ustate);
Review Comment:
Let's write a TDML file with some test cases and put it in a JIRA issue at
least.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/RunnerFactory.scala:
##########
@@ -37,28 +39,48 @@ import org.apache.daffodil.util.Misc
*/
object Runner {
def apply(dir: String, file: String,
+ useTDMLImplementation: TDMLImplementation = TDMLImplementation.Daffodil,
validateTDMLFile: Boolean = true,
validateDFDLSchemas: Boolean = true,
compileAllTopLevel: Boolean = false,
defaultRoundTripDefault: RoundTrip = defaultRoundTripDefaultDefault,
defaultValidationDefault: String = defaultValidationDefaultDefault,
defaultImplementationsDefault: Seq[String] =
defaultImplementationsDefaultDefault): Runner = {
- val resourceDir = if (dir.startsWith("/")) dir else "/" + dir
- val resourcePath = if (resourceDir.endsWith("/")) resourceDir + file else
resourceDir + "/" + file
+ // Prepend forward slash to turn dir/file into classpath resource
Review Comment:
Good idea, I like the idea, but not enough time to make these changes and
test them well enough in this PR.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -194,7 +198,7 @@ class DFDLTestSuite private[tdml] (
defaultImplementationsDefault: Seq[String] =
Runner.defaultImplementationsDefaultDefault,
shouldDoErrorComparisonOnCrossTests: Boolean =
Runner.defaultShouldDoErrorComparisonOnCrossTests,
shouldDoWarningComparisonOnCrossTests: Boolean =
Runner.defaultShouldDoWarningComparisonOnCrossTests) =
- this(null, aNodeFileOrURL, validateTDMLFile, validateDFDLSchemas,
compileAllTopLevel,
+ this(null, aNodeFileOrURL, useTDMLImplementation, validateTDMLFile,
validateDFDLSchemas, compileAllTopLevel,
Review Comment:
You're right, this public deprecated constructor isn't called by Daffodil
anymore so I will remove it. To avoid breaking backward compatibility in case
there's any external code which still calls this deprecated constructor, I will
remove the extra `val __nl: Null,` parameter from the other constructor. We
can make the other constructor public if we need to, but I think Java code can
call a Scala private constructor.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/RunnerFactory.scala:
##########
@@ -37,28 +39,48 @@ import org.apache.daffodil.util.Misc
*/
object Runner {
def apply(dir: String, file: String,
+ useTDMLImplementation: TDMLImplementation = TDMLImplementation.Daffodil,
Review Comment:
Thanks, I didn't realize that `TDML_IMPLEMENTATION="daffodilC" sbt test....`
would not work with these current changes. Now the changes will let that
command work.
--
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]
