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

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

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



##########
File path: .github/workflows/main.yml
##########
@@ -147,13 +151,13 @@ jobs:
         uses: codecov/codecov-action@v1.5.2
 
       - name: Prepare for SonarCloud Scan
-        if: ${{ runner.os == 'Linux' && matrix.java_version == '11' && github.repository == 'apache/daffodil' }}
+        if: ${{ matrix.sonarscan == 'true' && (github.event_name == 'push' || github.event.pull_request.head.repo.full_name == 'apache/daffodil') }}
         run: |
           $SBT coverageReport
           find . -name scoverage.xml -exec sed -i 's#/home/runner/work/daffodil/daffodil#/github/workspace#g' {} +
 
       - name: Run SonarCloud Scan
-        if: ${{ runner.os == 'Linux' && matrix.java_version == '11' && github.repository == 'apache/daffodil' }}
+        if: ${{ matrix.sonarscan == 'true' && (github.event_name == 'push' || github.event.pull_request.head.repo.full_name == 'apache/daffodil') }}

Review comment:
       If we can define sonarscan using an expression, then we can reduce the if lines to simply:
   
           if: ${{ matrix.sonarscan == true }}
           if: ${{ matrix.sonarscan }} # or this might work too...
   
   

##########
File path: daffodil-tdml-processor/src/test/java/org/apache/daffodil/tdml/TestRunnerFactory.java
##########
@@ -33,9 +34,18 @@
 
   @Test
   public void testPrimaryConstructor() {
-    Runner runner = new Runner(null, testDir, testTdmlFile, true, true, false, NoRoundTrip$.MODULE$,
-        "off", JavaConverters.asScalaBufferConverter(Arrays.asList("daffodil", "ibm")).asScala());
+    URI tdmlUri = Misc.getRequiredResource("org/apache/daffodil/tdml/genericTdml.tdml");
+    scala.util.Right<scala.xml.Elem, String> rightURI = new scala.util.Right<scala.xml.Elem, String>(tdmlUri.toString());

Review comment:
       Is it possible to import scala.util.Right and write this line more simply as...?
   
       Right<Elem, String> rightURI = new Right<>(tdmlUri.toString());
   
   Java 7 or later allows the diamond (<>) to be used on the right side.

##########
File path: .github/workflows/main.yml
##########
@@ -45,6 +45,10 @@ jobs:
             shell: msys2 {0}
             env_cc: clang
             env_ar: llvm-ar
+          - os: ubuntu-20.04
+            java_version: 11
+            scala_version: 2.12.14
+            sonarscan: true

Review comment:
       I wonder if we could do away with the extra include and simply add the sonarscan to the existing ubuntu-20.04 include.  For that to work, we would need to define the sonarscan using an expression instead of a literal:
   
               sonarscan: ${{ matrix.java_version == 11 }}
   
   If that expression works, we could make the if statements even more concise by moving their remaining expression clauses into sonarscan's expression too:
   
               sonarscan: ${{ matrix.java_version == 11 && (github.event_name == 'push' || github.event.pull_request.head.repo.full_name == 'apache/daffodil') }}
               # or even shorter...
               sonarscan: ${{ matrix.java_version == 11 && secrets.SONAR_TOKEN != '' }}
   
   I have no idea whether these expressions would work, but it's worth a try.

##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section06/entities/TestEntitiesNew.scala
##########
@@ -1,32 +0,0 @@
-/*
- * 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.section06.entities
-
-import org.apache.daffodil.util._
-import org.apache.daffodil.tdml.DFDLTestSuite
-
-class TestEntitiesNew {
-  val testDir = "/org/apache/daffodil/section06/entities/"

Review comment:
       What are these *New.scala files that you deleted?  Were they redundant?

##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -468,6 +461,66 @@ class DFDLTestSuite private[tdml] (
     }
   }
 
+  /**
+   * The CompileResult cache is stored in the DFDLTestSuite instance. This
+   * means any tests run from the same instance can share the same compiled
+   * processor. Once the DFDLTestSuite goes out of scope and is garbage
+   * collected, so too will be the compiled processors. Running the tests again
+   * will require creating a new DFDLTestSuite instance, and thus will
+   * recompile the schema.
+   */
+  private val tdmlCompileResultCache = new mutable.HashMap[TDMLCompileResultCacheKey, TDML.CompileResult]()

Review comment:
       HashMap has a companion object with an apply method so I think you can remove the `new`.
   
   def apply[K, V](elems: (K, V)*): HashMap[K, V]
   A collection of type Map that contains given key/value bindings.

##########
File path: daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/RunnerFactory.scala
##########
@@ -87,79 +94,133 @@ object Runner {
 }
 
 /**
- * Needs to be thread-safe (i.e., use all thread-local state) so that
- * test can be run in parallel.
+ * A Runner is responsible for creating and running tests in a DFDLTestSuite. A
+ * Runner will lazily create a DFDLTestSuite associated with the parameters
+ * only when a test is run. Tests can be run in parallel, and synchronization
+ * will be used to ensure only a single DFDLTestSuite is created and used.
  *
- * 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 always
+   * works as a classpath resource.
+   */
+  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 appended
+   * between the dir and file if needed, and then we call the "path: String"
+   * constructor which will prepend a forward slash if needed. This ensures
+   * this dir + file path always works as a classpath resource.
+   */
+  def this(dir: String, file: String) =
+    this(
+      if (dir.endsWith("/")) dir + file
+      else dir + "/" + file
+    )
+
+  /**
+   * Create a runner for a URI.
+   */
+  def this(uri: java.net.URI) =
+    this(Right(uri.toString))
+
+  /**
+   * Create a runner for a File
    */
-  def this(dir: String, file:String) =
-    this(null, dir, file)
+  def this(file: java.io.File) =
+    this(file.toURI)
 
+  /**
+   * Create a runner for a scala XML Elem. This is generally only used for testing
+   */
   def this(elem: scala.xml.Elem) =
-    this(elem, null, null)
-
-  if (elem ne null)
-    Assert.usage((dir eq null) && (file eq null))
-  else
-    Assert.usage((dir ne null) && (file ne null))
-
-  private lazy val resource = {
-    // This is ok to be a hard-wired "/" because these are resource identifiers, which
-    // are not file-system paths that have to be made platform-specific.
-    // In other words, we don't need to use "\\" for windows here. "/" works there as well.
-    val d = if (dir.endsWith("/")) dir else dir + "/"
-    Misc.getRequiredResource(d + file)
-  }
+    this(Left(elem))
+
+
+  private var ts: DFDLTestSuite = null
 
-  private def getTS = {
+  // This Runner should only ever have a single DFDLTestSuite associated with
+  // it. But different tests using this runner could be run in parallel. We
+  // also do not want to create the runner until a test actually uses it to
+  // avoid unnecessary allocations/computations. So all access of the
+  // underlying test suite should occur through this function, which is
+  // synchronized to ensure we only ever create on per Runner.

Review comment:
       Typo: on -> one

##########
File path: daffodil-tdml-lib/src/test/scala/org/apache/daffodil/tdml/UnitTestTDMLRunner.scala
##########
@@ -245,32 +256,34 @@ class UnitTestTDMLRunner {
 
   @Test def testEmbeddedSchemaValidates(): Unit = {
     val testSuite = tdmlWithEmbeddedSchemaInvalid
-    assertFalse(ts.isTDMLFileValid)
-    lazy val ts = new DFDLTestSuite(testSuite)
+    val runner = new Runner(testSuite)
+    lazy val ts = runner.getTS

Review comment:
       Can't the lazy val be replaced here too?




-- 
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