[GitHub] [daffodil] stevedlawrence commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r660551968



##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/processor/TDMLDFDLProcessor.scala
##########
@@ -119,14 +119,24 @@ trait TDMLDFDLProcessor {
 
   def unparse(parseResult: TDMLParseResult, outStream: java.io.OutputStream): TDMLUnparseResult
 
+  /**
+   * The TDMLRunner may cache the TDMLDFDLProcessor so that it can be reused
+   * among multiple parse/unparse calls without needing to rebuild the
+   * processor. This function is called once the TDMLRunner is done with the
+   * TMLDFDLProcessor. This allows state such as temporary files to be cleaned
+   * up. Actually does nothing unless overridden by an implementation.
+   */
+  def cleanUp(): Unit = { /* Do nothing */ }

Review comment:
       This cleanUp function is called by the DFDLTestSuite.cleanUp method (called by Runner.reset), so I think it needs to be public.

##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/RunnerFactory.scala
##########
@@ -91,75 +98,127 @@ object Runner {
  * test can be run in parallel.
  *
  * Note however, that each thread will get its own copy of the DFDLTestSuite

Review comment:
       Much of this comment isn't true anymore. I'll update it.

##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/RunnerFactory.scala
##########
@@ -91,75 +98,127 @@ object Runner {
  * test can be run in parallel.
  *
  * Note however, that each thread will get its own copy of the DFDLTestSuite
+ *
+ * source is either a scala.xml.Elem, generally used for unit testing or a
+ * String. If the string starts with a / is it treated is if it were resoucrce
+ * on the classpath. Otherwise it is treated as if it were a URI.
  */
-class Runner private (elem: scala.xml.Elem, dir: String, file: String,
+class Runner private (
+  source: Either[scala.xml.Elem, String],

Review comment:
       Yeah, changes were made so schemas can no longer be passed in as an Elem, but the entire testSuite can still be an Elem. We could also remove this capability, but we'd have to convert all those inline TDML nodes to .tdml files. I'm not against that if we want to further clean this up, but it might be a bit more work. Thoughts?

##########
File path: daffodil-tdml-processor/src/test/scala/org/apache/daffodil/tdml/TestTDMLRunner.scala
##########
@@ -355,8 +408,9 @@ class TestTDMLRunner {
 
   @Test def testMultiByteUnicodeWorks(): Unit = {
     val testSuite = tdmlWithUnicode2028
-    lazy val ts = new DFDLTestSuite(testSuite)
-    ts.runOneTest("testMultiByteUnicodeWorks")
+    lazy val runner = new Runner(testSuite)

Review comment:
       Will do

##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/RunnerFactory.scala
##########
@@ -91,75 +98,127 @@ object Runner {
  * test can be run in parallel.
  *
  * Note however, that each thread will get its own copy of the DFDLTestSuite
+ *
+ * source is either a scala.xml.Elem, generally used for unit testing or a
+ * String. If the string starts with a / is it treated is if it were resoucrce
+ * on the classpath. Otherwise it is treated as if it were a URI.
  */
-class Runner private (elem: scala.xml.Elem, dir: String, file: String,
+class Runner private (
+  source: Either[scala.xml.Elem, String],
   validateTDMLFile: Boolean = true,
   validateDFDLSchemas: Boolean = true,
   compileAllTopLevel: Boolean = false,
   defaultRoundTripDefault: RoundTrip = Runner.defaultRoundTripDefaultDefault,
   defaultValidationDefault: String = Runner.defaultValidationDefaultDefault,
   defaultImplementationsDefault: Seq[String] = Runner.defaultImplementationsDefaultDefault) {
 
-  /*
-   * these constructors are for use by Java programs
+
+  /**
+   * Create a runner for the path. This constructor requires this path be on
+   * the classpath. A forward slash is prepended to ensure this path aways
+   * works as a classpath resource.
    */
-  def this(dir: String, file:String) =
-    this(null, dir, file)
+  def this(path: String) =
+    this(
+      if (path.startsWith("/")) Right(path)
+      else Right("/" + path)
+    )
 
+  /**
+   * Create a runner for the dir + file combination. This constructor requires
+   * this combined path be on the classpath. A forward slash is prepended to

Review comment:
       It actually ends up calling the `path: String` constructor which will prepend the slash if needed. When using strings with the constructors (either dir + file or path), we assume that it's a resource and so the prepended slash is required.  I'll update the comment to make it more clear. 

##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/processor/TDMLDFDLProcessor.scala
##########
@@ -119,14 +119,24 @@ trait TDMLDFDLProcessor {
 
   def unparse(parseResult: TDMLParseResult, outStream: java.io.OutputStream): TDMLUnparseResult
 
+  /**
+   * The TDMLRunner may cache the TDMLDFDLProcessor so that it can be reused
+   * among multiple parse/unparse calls without needing to rebuild the
+   * processor. This function is called once the TDMLRunner is done with the
+   * TMLDFDLProcessor. This allows state such as temporary files to be cleaned
+   * up. Actually does nothing unless overridden by an implementation.
+   */
+  def cleanUp(): Unit = { /* Do nothing */ }
 }
 
 trait TDMLResult {
   def isValidationError: Boolean
   def isProcessingError: Boolean
   def getDiagnostics: Seq[Diagnostic]
+
   /**
-   * Deletes any temporary files that have been generated.
+   * Called once the TDMLRunner has finished analyzing the parse/unparse
+   * result. This allows state such as temporary files to be cleaned up.
    * Actually does nothing unless overridden by an implementation.
    */
   def cleanUp(): Unit = { /* Do nothing */ }

Review comment:
       Similar to above, this is called when the Parser/UnparserTestCase if finished running the test, so needs to be public.

##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/RunnerFactory.scala
##########
@@ -91,75 +98,127 @@ object Runner {
  * test can be run in parallel.
  *
  * Note however, that each thread will get its own copy of the DFDLTestSuite
+ *
+ * source is either a scala.xml.Elem, generally used for unit testing or a
+ * String. If the string starts with a / is it treated is if it were resoucrce
+ * on the classpath. Otherwise it is treated as if it were a URI.
  */
-class Runner private (elem: scala.xml.Elem, dir: String, file: String,
+class Runner private (
+  source: Either[scala.xml.Elem, String],
   validateTDMLFile: Boolean = true,
   validateDFDLSchemas: Boolean = true,
   compileAllTopLevel: Boolean = false,
   defaultRoundTripDefault: RoundTrip = Runner.defaultRoundTripDefaultDefault,
   defaultValidationDefault: String = Runner.defaultValidationDefaultDefault,
   defaultImplementationsDefault: Seq[String] = Runner.defaultImplementationsDefaultDefault) {
 
-  /*
-   * these constructors are for use by Java programs
+
+  /**
+   * Create a runner for the path. This constructor requires this path be on
+   * the classpath. A forward slash is prepended to ensure this path aways
+   * works as a classpath resource.
    */
-  def this(dir: String, file:String) =
-    this(null, dir, file)
+  def this(path: String) =
+    this(
+      if (path.startsWith("/")) Right(path)
+      else Right("/" + path)

Review comment:
       Yeah, It's very possible other DFDL projects might hit this. It wouldn't be too hard to add some tests to make sure all these constructors are covered and work correctly. Will add.

##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/unparser/TestOVCAndLength.scala
##########
@@ -19,14 +19,10 @@ package org.apache.daffodil.unparser
 
 import org.junit.Test
 import org.junit.AfterClass
-import org.apache.daffodil.util._
-import org.apache.daffodil.tdml.DFDLTestSuite
+import org.apache.daffodil.tdml.Runner
 
 object TestOVCAndLength {
-  val testDir = "/org/apache/daffodil/unparser/"
-  val aa = testDir + "OVCAndLengthTest.tdml"
-  var runner = new DFDLTestSuite(Misc.getRequiredResource(aa))
-
+  var runner = Runner("/org/apache/daffodil/unparser/OVCAndLengthTest.tdml")
   @AfterClass def tearDown(): Unit = {
     runner = null

Review comment:
       Raises a minor point. Should all these be var's and we set them to null after the reset? Each Runner instance itself doesn't take up much memory once it's reset. It's really just the constructor arguments and and the memory for the Runner itself, so maybe it's not worth it? But I guess this is technically a leak since these Runner's can never be cleaned up? Maybe we want runner.reset to return null so that you could do something like:
   
   ```scala
   @AfterClass def tearDown(): Unit = {
       runner = runner.reset
   }
   ```
   So we call reset to free up cached stuff and delete any tempoary files, and then set the runner to null the the runner instance itself can be free?
   
   Though, what happens if you re-run the test? Isn't the runner null? Or doe var runner somehow get reinitialized? How do we not get a null pointer exception when this test is rerun?




-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org