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 2021/06/28 19:43:12 UTC
[GitHub] [daffodil] stevedlawrence opened a new pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
stevedlawrence opened a new pull request #599:
URL: https://github.com/apache/daffodil/pull/599
- With the compiled processor cache stored in a global object, it meant
the cache would never be emptied. Although this avoids schema
compilation, it also meant TDML runners could use a large amount of
memory that could never be released. This changes the TDML runner so
that the cache is stored on the DFDLTestSuite. This means only tests
run from the same runner can share compiled schemas. This also means
once a test suite has completed and goes out of scope, so do those
compiled schemas. So re-running a test requires recompliation. To
ensure we only compile once even when parllel threads might access the
same Runner/DFDLTestSuite, we add syncrhonization during schema
compliation.
- This also removes the existing SchemaCache. It had some logic that we
no longer need anymore (e.g. file change detection) since we now
recompile schemas more frequently. The new cache also supports caching
results from any implementation rather than from just the Daffodil
runtime1 implementation.
- This also removes a ThreadLocal from the RunnerFactory to ensure
different threads using the same Runner do not recreate a dupliate
DFDLTestSuite. We want to share DFDLTestSuite's as much as possible.
This does now mean that parse/unparse can actually happen in parelell
using the same CompileResult. If an implementation does not support
that, they must implement their own syncrhonization during these
calls.
- With runtime2 processor cached and shared among individual
parse/unparse tests, this revealed a bug where we cleaned up the
generated executable incorrectly. We should only delete the generated
executable when we were done with the CompileResult, but we instead
deleted it whenever we finished an individual test. This meant that if
two tests shared a processor, the first test would delete the
executable and it wouldn't exist for the second test. To fix this, we
add a new cleanUp method to the TDMLDFDLProcessor, which is called
when the DFDLTestSuite is reset to empty. The parse/unparse results do
not actually need to clean anything up for runtime2 since they clean
up their own temporary data.
- Also adds prefixies to runtime2 temp files so that it's easier to know
which temp files belong to Daffodil.
- Remove all uses of DFDLTestSuite and deprecate the only public
constructor. Replace all instances of the DFDLTestSuite with the
Runner and call reset to ensure temp files are cleaned up.
- Remove the ability to pass in a schema node to TDML Runner functions.
This was only used for unit tests. This instead changes unit tests to
used things like embedded TDML schemas so that these unit tests use
the same code path as normal tests.
DAFFODIL-2520
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r661654927
##########
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:
Actually, reading through this:
https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
My suggestion is a bad idea. Malicious users are potentially able to access the secrets. But that post does mention how to do what we want safely. I think this is getting to get complicated and outside of the scope of this PR, so I think that's best handled in a separate PR.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r661650533
##########
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:
For reference, I think the change would look like this:
```scala
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index f187d05ef..1547a3243 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -20,7 +20,7 @@ name: Daffodil CI
on:
push:
- pull_request:
+ pull_request_target:
types: [opened, synchronize, reopened]
# Build Daffodil and run some checks.
@@ -57,9 +57,7 @@ jobs:
matrix.os == 'ubuntu-20.04' &&
matrix.java_version == '11' &&
matrix.scala_version == '2.12.14' &&
- github.event_name == 'push' &&
- github.repository == 'apache/daffodil' &&
- github.ref == 'refs/heads/master'
+ github.repository == 'apache/daffodil'
}}
steps:
```
So we change pull_request to pull_request_target, and enable sonarcloud for anything run in the apache/daffodil repository, whether it be a branch, or a pull request.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r661414144
##########
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:
Yep, I think this is the way to do it. It looks like expressions can be used in include values, but `matrix` doesn't exist in. PR updated.
Some things I noticed
1. This entire main.yml was changed to only run on pushes to master, instead of all pushes. This meant that if you pushed a branch to your own fork then checks would not run unless you pushed to your forks master branch. So you couldn't tell if checks passed without opening a PR. I found that useful sometimes, so changed it to run on pushes to any branch. Still runs on all PRs too.
2. I changed sonarscan to only run on pushes to the master branch in the apache/daffodil repo. It will not run on any PRs and not on any other branches. Sonar cloud isn't going to work on 99% of PRs since they all come from forks. The only one it will work on is pushes to dev branches (which are rare now that runtime2 is merged) and dependabot (which are likely never going to have sonarcloud findings). So this just simplifies the config. I also chose to not check against SONAR_TOKEN, because other forks could potentially add their own SONAR_TOKEN, but that won't work with our sonarproject.properties file, which specifies the apache/daffodil project. So, unfortunately, I think sonarscan really only makes sense to run when master is updated. At least until they fix it so PRs can run scans.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r660888700
##########
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:
A long time ago, well before Apache, we had a workflow where developers would add tests to a `FooNew.scala` file and then when a tester verified the test they would move the test to the `Foo.scala` file. I think the idea was that we could easily differentiate between which tests had been verified and which hadn't and track progress. As time went on, testing resources decreased and so we couldn't really maintain this workflow and it didn't really add all that much, so we don't do that anymore. Some of those "New" files still exist, but don't actually have any tests in them because all the tests had been verified and moved out. I'll do another scan for any empty New files and delete them if they're empty. I only noticed these because they used the now deprecated DFDLTestSuite
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r660890174
##########
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:
I'm not sure if that's possible, I can't find the syntax for what `include` accepts but I don't see any examples anywhere where it's set to an expression. Might be some other way to do this though? Having one boolean value would be nice.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by 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
[GitHub] [daffodil] tuxji commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r660910169
##########
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:
> I'm not sure if that's possible, I can't find the syntax for what `include` accepts but I don't see any examples anywhere where it's set to an expression. Might be some other way to do this though? Having one boolean value would be nice.
Yes, there is another way we can do this. We can add an env section after the matrix section and define sonarscan there instead of in the matrix section:
env:
SONARSCAN: ${{ long expression ... }}
- name: Run SonarCloud Scan
if: ${{ env.SONARSCAN == 'true' }}
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r660891388
##########
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'll give it a shot and see what happens. Note the we should also check the scala_version in case we ever start cross compiling and add another scala version to the matrix.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r661600599
##########
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:
I just came across this the "pull_request_target":
https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target
It's the same as `on: pull_request`, but you get access to secrets with the restriction that it uses the actions file from the base of the PR (which is trusted) instead of in the PR. So we could switch to this and then we could renable sonarcloud for everything. The only downside is it becomes a bit harder to test github actions changes in a PR since they'll only take affect once merged. But we change that file so rarely that it's probably worth the trouble.
Thoughts?
--
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
[GitHub] [daffodil] tuxji commented on pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
tuxji commented on pull request #599:
URL: https://github.com/apache/daffodil/pull/599#issuecomment-870055334
Okay, now I see why the "Set the SONAR_TOKEN env variable" error message occurs. When a pull request comes from a forked repository, the fork's job is not allowed to use any secret from the daffodil repository. It still can use secrets.GITHUB_TOKEN (that's not really a secret because the forked repository has a github_token automatically) but it can't use secrets.SONAR_TOKEN. We need to change the if condition for the Sonar steps to something like
` if: ${{ runner.os == 'Linux' && matrix.java_version == '11' && (github.event_name == 'push' || github.event.pull_request.head.repo.full_name == 'apache/daffodil') }}`
If you make that change to two lines in main.yml (lines 150 and 156), the CI should skip the Sonar steps for your pull request and all your checks should pass.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r660077349
##########
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:
Is this Note above still true? They all share one which synchronizes the schema compile method don't they?
##########
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:
Should be protected method?
##########
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:
You said you got rid of support for using an Elem here. Do we still need this Either then?
Ah. I see you still allow the whole test suite to be an Elem, just not a schema.
##########
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:
Hmmm. Not sure I would add tests showing this auto pre-slashifies the path. If this isn't covered I think I'd rather change this to assert that the path must startwith "/" and fail otherwise.
OTOH. I guess others may have TDML files that depend on this behavior. So I guess a test would be good. You can just modify some other test this way, and have that test also exercise this little feature.
##########
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")
Review comment:
should be val
##########
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:
This comment is wrong. It's a cut paste from prior method doc. This forward slash is appended if needed, but it doesn't have to do with classpath. Just has to do with making sure the dir and filename are separated by a "/".
##########
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:
protected?
##########
File path: daffodil-tdml-processor/src/main/scala/org/apache/daffodil/tdml/processor/Runtime2TDMLDFDLProcessor.scala
##########
@@ -190,20 +191,24 @@ class Runtime2TDMLDFDLProcessor(tempDir: os.Path, executable: os.Path,
// the unparsed data on its standard output, and write any error messages
// on its standard output (all done in [[Runtime2DataProcessor.unparse]]).
override def unparse(infosetXML: scala.xml.Node, outStream: java.io.OutputStream): TDMLUnparseResult = {
- val tempInputFile = XMLUtils.convertNodeToTempFile(infosetXML, tempDir.toIO)
- val inStream = os.read.inputStream(os.Path(tempInputFile))
+ val inStream = new ByteArrayInputStream(infosetXML.toString.getBytes())
val upr = dataProcessor.unparse(inStream, outStream)
anyErrors = upr.isError
diagnostics = diagnostics ++ upr.getDiagnostics
- new Runtime2TDMLUnparseResult(upr, tempDir)
+ new Runtime2TDMLUnparseResult(upr)
}
def unparse(parseResult: TDMLParseResult, outStream: java.io.OutputStream): TDMLUnparseResult = {
unparse(parseResult.getResult, outStream)
}
+
+ /**
+ * Remove the generated executable
+ */
+ override def cleanUp(): Unit = os.remove.all(tempDir)
Review comment:
override protected def ?
##########
File path: daffodil-tdml-processor/src/test/scala/org/apache/daffodil/tdml/TestTDMLRunner.scala
##########
@@ -68,34 +69,30 @@ class TestTDMLRunner {
</parserTestCase>
</testSuite>
- using(new java.io.FileWriter(tmp)) {
- fileWriter =>
- fileWriter.write(testSchema.toString())
- }
-
- // compileAllTopLevel must be true to enable caching
- lazy val ts = new DFDLTestSuite(testSuite, true, true, true)
+ lazy val runner = new Runner(testSuite)
// Run the non-validation test first, this will compile, serialize, and
// cache the schema
- ts.runOneTest("test3a")
+ runner.runOneTest("test3a")
// Run the validation test second. This should recompile and cache the
// schema separately from from the first test. If it ended up using the
// non-validation cached Processor, an exception would be thrown since it
// was serialized and that does not work with validation="on"
- ts.runOneTest("test3b")
- }
+ runner.runOneTest("test3b")
- val testSchema = SchemaUtils.dfdlTestSchema(
- <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>,
- <dfdl:format ref="tns:GeneralFormat"/>,
- <xs:element name="data" type="xs:int" dfdl:lengthKind="explicit" dfdl:length="{ xs:unsignedInt(2) }"/>)
+ runner.reset
+ }
@Test def testTDMLParseSuccess(): Unit = {
- val testSuite = <ts:testSuite xmlns:ts={ tdml } suiteName="theSuiteName">
- <ts:parserTestCase ID="some identifier" name="testTDMLParseSuccess" root="data">
+ val testSuite = <ts:testSuite xmlns:ts={ tdml } suiteName="theSuiteName" xmlns:xs={ xsd } xmlns:dfdl={ dfdl } xmlns:tns={ example }>
+ <ts:defineSchema name="mySchema">
Review comment:
This is a nice cleanup, as all these tests pre-dated our introduction of the ts:defineSchema element to tdml.
##########
File path: daffodil-tdml-processor/src/test/scala/org/apache/daffodil/tdml/TestTDMLRunnerCommentSyntax.scala
##########
@@ -87,8 +87,9 @@ class TestTDMLRunnerCommentSyntax {
<!-- comment -->
</tdml:testSuite>
- lazy val ts = new DFDLTestSuite(testSuite)
- ts.runOneTest("test1")
+ lazy val runner = new Runner(testSuite)
Review comment:
not lazy
##########
File path: daffodil-tdml-processor/src/test/scala/org/apache/daffodil/tdml/TestTDMLRunner.scala
##########
@@ -509,16 +566,16 @@ f0 f1 f2 f3 f4 f5 f6 f7 f8 f9 fa fb fc fd fe ff
</tdml:parserTestCase>
</tdml:testSuite>
- lazy val ts = new DFDLTestSuite(testSuite)
- val bytes = ts.parserTestCases(0).document.get.documentBytes
+ lazy val runner = new Runner(testSuite)
+ val bytes = runner.getTS.parserTestCases(0).document.get.documentBytes
assertEquals(9, bytes.length)
- ts.runOneTest("testNilCompare")
+ runner.runOneTest("testNilCompare")
}
@Test def test_tdmlNamespaces1(): Unit = {
val testDir = "/test/tdml/"
val t0 = testDir + "tdmlNamespaces.tdml"
- lazy val r = new DFDLTestSuite(Misc.getRequiredResource(t0))
+ lazy val r = new Runner(t0)
Review comment:
Not lazy val
##########
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:
should be runner.reset
##########
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:
These don't need to be lazy val. You can probably global replace ```lazy val runner =``` with ```val runner =```
--
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
[GitHub] [daffodil] tuxji commented on pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
tuxji commented on pull request #599:
URL: https://github.com/apache/daffodil/pull/599#issuecomment-870668309
> Does it make sense to move Sonarcloud back to it's own file? Our workflow pretty much always uses forks (except for the dependabot things which we can't change) so pull requests are rarely going to run for pull requests.
It doesn't really make sense to move Sonar back to sonarcloud.yml. The Sonar scanner needs to see the fully built source tree and we need to run tests for coverage information anyway, so we do that only once in main.yml and then call the Sonar scanner as an afterthought if appropriate. We just have to avoid running the Sonar scanner when we don't have secrets.SONAR_TOKEN available. I'll look for a way to make the if condition more concise later, but we're still benefitting from not having to duplicate anything between main.yml and sonarcloud.yml now.
> Also, this PR branched before the sonarcloud changes were merged. So this PR shouldn't even have updated main.yml? Do github actions the main.yml from the master branch now and not from the PR?
The workflow definitely used the updated main.yml. I believe GitHub must have merged the PR's changes into the master branch when checking out the source code for the workflow. That's why secrets.GITHUB_TOKEN still works, but not secrets.SONAR_TOKEN.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r660596518
##########
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:
I would not further change this. It is very convenient and a big advantage of Scala that one can just create Nodes for testing directly in a unit test. Much more convenient than dealing with separate file processing.
##########
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:
Not worth it. Making runner reset return null like that is a fine change, but we're not worried about regression testing Daffodil running TDML tests in a loop indefinitely. That's not an application scenario we care about. We have a few thousand tests, suppose that grows to 100K tests, and there's average 10 tests per runner, so 10K runners. If each is a 10K object that's only 10Mbytes.
##########
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:
Is this Note above still true? They all share one which synchronizes the schema compile method don't they?
##########
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:
Should be protected method?
##########
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:
You said you got rid of support for using an Elem here. Do we still need this Either then?
Ah. I see you still allow the whole test suite to be an Elem, just not a schema.
##########
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:
Hmmm. Not sure I would add tests showing this auto pre-slashifies the path. If this isn't covered I think I'd rather change this to assert that the path must startwith "/" and fail otherwise.
OTOH. I guess others may have TDML files that depend on this behavior. So I guess a test would be good. You can just modify some other test this way, and have that test also exercise this little feature.
##########
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")
Review comment:
should be val
##########
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:
This comment is wrong. It's a cut paste from prior method doc. This forward slash is appended if needed, but it doesn't have to do with classpath. Just has to do with making sure the dir and filename are separated by a "/".
##########
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:
protected?
##########
File path: daffodil-tdml-processor/src/main/scala/org/apache/daffodil/tdml/processor/Runtime2TDMLDFDLProcessor.scala
##########
@@ -190,20 +191,24 @@ class Runtime2TDMLDFDLProcessor(tempDir: os.Path, executable: os.Path,
// the unparsed data on its standard output, and write any error messages
// on its standard output (all done in [[Runtime2DataProcessor.unparse]]).
override def unparse(infosetXML: scala.xml.Node, outStream: java.io.OutputStream): TDMLUnparseResult = {
- val tempInputFile = XMLUtils.convertNodeToTempFile(infosetXML, tempDir.toIO)
- val inStream = os.read.inputStream(os.Path(tempInputFile))
+ val inStream = new ByteArrayInputStream(infosetXML.toString.getBytes())
val upr = dataProcessor.unparse(inStream, outStream)
anyErrors = upr.isError
diagnostics = diagnostics ++ upr.getDiagnostics
- new Runtime2TDMLUnparseResult(upr, tempDir)
+ new Runtime2TDMLUnparseResult(upr)
}
def unparse(parseResult: TDMLParseResult, outStream: java.io.OutputStream): TDMLUnparseResult = {
unparse(parseResult.getResult, outStream)
}
+
+ /**
+ * Remove the generated executable
+ */
+ override def cleanUp(): Unit = os.remove.all(tempDir)
Review comment:
override protected def ?
##########
File path: daffodil-tdml-processor/src/test/scala/org/apache/daffodil/tdml/TestTDMLRunner.scala
##########
@@ -68,34 +69,30 @@ class TestTDMLRunner {
</parserTestCase>
</testSuite>
- using(new java.io.FileWriter(tmp)) {
- fileWriter =>
- fileWriter.write(testSchema.toString())
- }
-
- // compileAllTopLevel must be true to enable caching
- lazy val ts = new DFDLTestSuite(testSuite, true, true, true)
+ lazy val runner = new Runner(testSuite)
// Run the non-validation test first, this will compile, serialize, and
// cache the schema
- ts.runOneTest("test3a")
+ runner.runOneTest("test3a")
// Run the validation test second. This should recompile and cache the
// schema separately from from the first test. If it ended up using the
// non-validation cached Processor, an exception would be thrown since it
// was serialized and that does not work with validation="on"
- ts.runOneTest("test3b")
- }
+ runner.runOneTest("test3b")
- val testSchema = SchemaUtils.dfdlTestSchema(
- <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>,
- <dfdl:format ref="tns:GeneralFormat"/>,
- <xs:element name="data" type="xs:int" dfdl:lengthKind="explicit" dfdl:length="{ xs:unsignedInt(2) }"/>)
+ runner.reset
+ }
@Test def testTDMLParseSuccess(): Unit = {
- val testSuite = <ts:testSuite xmlns:ts={ tdml } suiteName="theSuiteName">
- <ts:parserTestCase ID="some identifier" name="testTDMLParseSuccess" root="data">
+ val testSuite = <ts:testSuite xmlns:ts={ tdml } suiteName="theSuiteName" xmlns:xs={ xsd } xmlns:dfdl={ dfdl } xmlns:tns={ example }>
+ <ts:defineSchema name="mySchema">
Review comment:
This is a nice cleanup, as all these tests pre-dated our introduction of the ts:defineSchema element to tdml.
##########
File path: daffodil-tdml-processor/src/test/scala/org/apache/daffodil/tdml/TestTDMLRunnerCommentSyntax.scala
##########
@@ -87,8 +87,9 @@ class TestTDMLRunnerCommentSyntax {
<!-- comment -->
</tdml:testSuite>
- lazy val ts = new DFDLTestSuite(testSuite)
- ts.runOneTest("test1")
+ lazy val runner = new Runner(testSuite)
Review comment:
not lazy
##########
File path: daffodil-tdml-processor/src/test/scala/org/apache/daffodil/tdml/TestTDMLRunner.scala
##########
@@ -509,16 +566,16 @@ f0 f1 f2 f3 f4 f5 f6 f7 f8 f9 fa fb fc fd fe ff
</tdml:parserTestCase>
</tdml:testSuite>
- lazy val ts = new DFDLTestSuite(testSuite)
- val bytes = ts.parserTestCases(0).document.get.documentBytes
+ lazy val runner = new Runner(testSuite)
+ val bytes = runner.getTS.parserTestCases(0).document.get.documentBytes
assertEquals(9, bytes.length)
- ts.runOneTest("testNilCompare")
+ runner.runOneTest("testNilCompare")
}
@Test def test_tdmlNamespaces1(): Unit = {
val testDir = "/test/tdml/"
val t0 = testDir + "tdmlNamespaces.tdml"
- lazy val r = new DFDLTestSuite(Misc.getRequiredResource(t0))
+ lazy val r = new Runner(t0)
Review comment:
Not lazy val
##########
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:
should be runner.reset
##########
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:
These don't need to be lazy val. You can probably global replace ```lazy val runner =``` with ```val runner =```
##########
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:
I would not further change this. It is very convenient and a big advantage of Scala that one can just create Nodes for testing directly in a unit test. Much more convenient than dealing with separate file processing.
##########
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:
Not worth it. Making runner reset return null like that is a fine change, but we're not worried about regression testing Daffodil running TDML tests in a loop indefinitely. That's not an application scenario we care about. We have a few thousand tests, suppose that grows to 100K tests, and there's average 10 tests per runner, so 10K runners. If each is a 10K object that's only 10Mbytes.
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #599:
URL: https://github.com/apache/daffodil/pull/599#issuecomment-870568709
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r661497980
##########
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:
+1 from me. Very nice changes!
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by 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
[GitHub] [daffodil] tuxji commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #599:
URL: https://github.com/apache/daffodil/pull/599#discussion_r661687192
##########
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:
Yes, "pull_request_target" appears to have some problems. I agree any attempt to enable SonarCloud for everything should be handled in a separate PR that tries to avoid these problems:
1) Attacker can inject code into the build.sbt files which runs with write permission to the repository (according to what that article said).
2) At the same time, our own PRs won't be able to test changes to the workflow files until the PR's merged (according to what you said).
By the way, I realized that running main.yml on pushes to any branch will cause duplicate checks to be run on pushes to dependabot branches since they always are associated with both branches and pull requests at the same time. However, we can explicitly ignore dependabot branches while allowing all other branches to trigger checks:
```yml
on:
push:
branches-ignore: [ 'dependabot/**' ]
pull_request:
types: [ opened, synchronize, reopened ]
```
--
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
[GitHub] [daffodil] tuxji commented on pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
tuxji commented on pull request #599:
URL: https://github.com/apache/daffodil/pull/599#issuecomment-870055334
Okay, now I see why the "Set the SONAR_TOKEN env variable" error message occurs. When a pull request comes from a forked repository, the fork's job is not allowed to use any secret from the daffodil repository. It still can use secrets.GITHUB_TOKEN (that's not really a secret because the forked repository has a github_token automatically) but it can't use secrets.SONAR_TOKEN. We need to change the if condition for the Sonar steps to something like
` if: ${{ runner.os == 'Linux' && matrix.java_version == '11' && (github.event_name == 'push' || github.event.pull_request.head.repo.full_name == 'apache/daffodil') }}`
If you make that change to two lines in main.yml (lines 150 and 156), the CI should skip the Sonar steps for your pull request and all your checks should pass.
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #599:
URL: https://github.com/apache/daffodil/pull/599#issuecomment-870752788
I think I've addressed all comments, including the SonarCloud scan updates. I've made it slightly less verbose and more directly tied to the specific matrix combination that it should run as. I think this way if we ever update the matrix, it will be more obvious that we also need to update the sonarcloud matrix thing.
--
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
[GitHub] [daffodil] stevedlawrence merged pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #599:
URL: https://github.com/apache/daffodil/pull/599
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #599:
URL: https://github.com/apache/daffodil/pull/599#issuecomment-870568709
> When a pull request comes from a forked repository, the fork's job is not allowed to use any secret from the daffodil repository.
Does it make sense to move Sonarcloud back to it's own file? Our workflow pretty much always uses forks (except for the dependabot things which we can't change) so pull requests are rarely going to run for pull requests.
Also, this PR branched before the sonarcloud changes were merged. So this PR shouldn't even have updated main.yml? Do github actions the main.yml from the master branch now and not from the PR?
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #599: Move TDML processor cache from a global object to DFDLTestSuite instances
Posted by 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