tuxji commented on a change in pull request #539:
URL: https://github.com/apache/daffodil/pull/539#discussion_r623373580
##########
File path: build.sbt
##########
@@ -15,8 +15,8 @@
* limitations under the License.
*/
-import sbt.io.Path.flatRebase
Review comment:
Thanks for noticing this is no longer needed.
##########
File path:
daffodil-core/src/test/scala/org/apache/daffodil/api/TestParseIndividualMessages.scala
##########
@@ -0,0 +1,192 @@
+/*
+ * 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.api
+
+import org.apache.daffodil.Implicits.intercept
+import org.apache.daffodil.io.SocketPairTestRig
+import org.apache.daffodil.util.SchemaUtils
+import org.apache.daffodil.util.TestUtils
+import org.junit.Assert._
+import org.junit.Test
+
+import java.io.InputStream
+import java.io.OutputStream
+import scala.concurrent.Await
+import scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent.Future
+import scala.concurrent.TimeoutException
+import scala.concurrent.duration.DurationInt
+import scala.xml.Node
+
+
+/**
+ * Shows that we can parse exactly 1 message from a TCP network socket
+ * without blocking for bytes past the end of the messsage.
+ *
+ * This only works for DFDL schemas of formats that are specified length.
+ */
+class TestParseIndividualMessages {
+
+ import SocketPairTestRig._
+
+ //
+ // DFDL schema for element e1 which occupies exactly 4 bytes.
+ //
+ val exactly4ByteSch = SchemaUtils.dfdlTestSchema(
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>,
+ <dfdl:format representation="binary" byteOrder="bigEndian"
binaryNumberRep="binary" ref="tns:GeneralFormat"/>,
+ <xs:element name="e1" type="xs:string" dfdl:lengthKind="explicit"
dfdl:length="4"/>)
+
+ /**
+ * Test shows that at least for simple fixed-length data, Daffodil parse
returns
+ * without requiring more bytes to be read than the exact length required.
+ */
+ @Test def testDaffodilParseFromNetwork1(): Unit = {
+ val sptr = new SocketPairTestRig {
+ override def test(pos: OutputStream, cis: InputStream): Unit = {
+
+ val dp = TestUtils.compileSchema(exactly4ByteSch)
+
+ //
+ // Write exactly 4 bytes to producer network stream
+ //
+ pos.write("1234".getBytes)
+ pos.flush()
+
+ //
+ // Daffodil parse element e1 from consumer input stream
+ //
+ // If we need more than 4 bytes to successfully parse (we shouldn't
for this schema)
+ // then this will hang, because only 4 bytes are in fact available.
+ //
+ // Caution: if debugging, this will timeout if you stop inside here!
+ //
+ val (pr: DFDL.ParseResult, xml: Node) =
+ withTimeout("Daffodil parse") {
+ TestUtils.runDataProcessorOnInputStream(dp, cis, areTracing = false)
+ }
+
+ assertFalse(pr.isError)
+ assertEquals("1234", xml.text)
+ assertEquals(33, pr.resultState.currentLocation.bitPos1b)
+ }
+ }
+ sptr.run()
+ }
+
+ //
+ // DFDL schema for delimited element.
+ //
+ private def delimitedSchema(term: String) = SchemaUtils.dfdlTestSchema(
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>,
+ <dfdl:format representation="text" ref="tns:GeneralFormat"/>,
+ <xs:element name="e1" type="xs:string" dfdl:lengthKind="delimited"
+ dfdl:terminator={ term } />)
+
+ /**
+ * Helper so we can test various delimiter-oriented scenarios.
+ *
+ * @param stringData Data to write. Should be small enough that the
parse will block.
+ * @param terminator String to use as terminating delimiter of
element. Can be more than one delimiter.
+ * @param followingDataString Data to write which should unblock the parse.
+ */
+ private def testHelperDaffodilParseDelimitedFromNetwork(
+ data: String,
+ terminator: String,
+ followingDataString: String) = {
+ val sptr = new SocketPairTestRig {
+ override def test(pos: OutputStream, cis: InputStream): Unit = {
+
+ val dp = TestUtils.compileSchema(delimitedSchema(terminator))
+
+ // Write the data. Should be too short to satisfy the parse.
+ //
+ pos.write(data.getBytes)
+ pos.flush()
+
+ val fut = Future {
+ TestUtils.runDataProcessorOnInputStream(dp, cis, areTracing = false)
+ }
+
+ Thread.sleep(100)
+ assertFalse(fut.isCompleted)
+ //
+ // Writing additional character(s) should unblock the parse.
+ //
+ pos.write(followingDataString.getBytes)
+ pos.flush()
+ val (pr, xml) = Await.result(fut, 100.milliseconds)
+ if (!pr.isError) {
+ assertEquals("1234", xml.text)
+ } else {
+ //parse failed.
+ val diagString = pr.getDiagnostics.map {
+ _.getMessage()
+ }.mkString("\n")
+ fail("Parse failed, but did not time-out.\n" + diagString)
+ }
+ }
+ }
+ sptr.run()
+ }
+
+ /**
+ * Test shows that for delimited data, we block seeking more data,
+ * but once the need for a terminator with longest match is satisfied
+ * the parse is unblocked.
+ */
+ // @Test // DAFFODIL-2504 - this fails looking for more data than is needed
for terminator.
Review comment:
If this test is supposed to work, let's change the scaladoc to say so
like you did for the next test: "This test does not work, but it should work.
Test shows that...." However, I see a fail("Test fails") which doesn't agree
with the comment - one of them must be out of date.
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/Runtime2DataProcessor.scala
##########
@@ -188,11 +196,13 @@ final class ParseResult(outfile: os.Path,
}
final class UnparseResult(val finalBitPos0b: Long,
- override val processorStatus: ProcessorResult,
- loc: DataLocation = Runtime2DataLocation())
+ override val processorStatus: ProcessorResult)
extends DFDL.UnparseResult
with DFDL.State
with WithDiagnosticsImpl {
+
+ val loc: DataLocation = Runtime2DataLocation(finalBitPos0b + 1,
(finalBitPos0b + 1) / 8)
+
/**
Review comment:
Thanks for the heads up and fixing those locations. You're setting them
to the end of the input (why add 1, though?).
##########
File path:
daffodil-io/src/test/scala/org/apache/daffodil/layers/TestBase64.scala
##########
@@ -92,7 +91,7 @@
W1vdXMgcXVvdGVzIG9yIHNvbmcgbHlyaWNzIG9yIGFueXRoaW5nIGxpa2UgdGhhdCBpbnRyb2R1Y
val data = b64Text.dropRight(3)
intercept[IllegalArgumentException] {
- println(new String(java.util.Base64.getMimeDecoder.decode(data)))
+ new String(java.util.Base64.getMimeDecoder.decode(data))
Review comment:
What are you trying to do here? From a Java programmer's perspective,
"new String(...)" sometimes is redundant.
##########
File path:
daffodil-core/src/test/scala/org/apache/daffodil/api/TestParseIndividualMessages.scala
##########
@@ -0,0 +1,192 @@
+/*
+ * 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.api
+
+import org.apache.daffodil.Implicits.intercept
+import org.apache.daffodil.io.SocketPairTestRig
+import org.apache.daffodil.util.SchemaUtils
+import org.apache.daffodil.util.TestUtils
+import org.junit.Assert._
+import org.junit.Test
+
+import java.io.InputStream
+import java.io.OutputStream
+import scala.concurrent.Await
+import scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent.Future
+import scala.concurrent.TimeoutException
+import scala.concurrent.duration.DurationInt
+import scala.xml.Node
+
+
+/**
+ * Shows that we can parse exactly 1 message from a TCP network socket
+ * without blocking for bytes past the end of the messsage.
+ *
+ * This only works for DFDL schemas of formats that are specified length.
+ */
+class TestParseIndividualMessages {
+
+ import SocketPairTestRig._
+
+ //
+ // DFDL schema for element e1 which occupies exactly 4 bytes.
+ //
+ val exactly4ByteSch = SchemaUtils.dfdlTestSchema(
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>,
+ <dfdl:format representation="binary" byteOrder="bigEndian"
binaryNumberRep="binary" ref="tns:GeneralFormat"/>,
+ <xs:element name="e1" type="xs:string" dfdl:lengthKind="explicit"
dfdl:length="4"/>)
+
+ /**
+ * Test shows that at least for simple fixed-length data, Daffodil parse
returns
+ * without requiring more bytes to be read than the exact length required.
+ */
+ @Test def testDaffodilParseFromNetwork1(): Unit = {
+ val sptr = new SocketPairTestRig {
+ override def test(pos: OutputStream, cis: InputStream): Unit = {
+
+ val dp = TestUtils.compileSchema(exactly4ByteSch)
+
+ //
+ // Write exactly 4 bytes to producer network stream
+ //
+ pos.write("1234".getBytes)
+ pos.flush()
+
+ //
+ // Daffodil parse element e1 from consumer input stream
+ //
+ // If we need more than 4 bytes to successfully parse (we shouldn't
for this schema)
+ // then this will hang, because only 4 bytes are in fact available.
+ //
+ // Caution: if debugging, this will timeout if you stop inside here!
+ //
+ val (pr: DFDL.ParseResult, xml: Node) =
+ withTimeout("Daffodil parse") {
+ TestUtils.runDataProcessorOnInputStream(dp, cis, areTracing = false)
+ }
+
+ assertFalse(pr.isError)
+ assertEquals("1234", xml.text)
+ assertEquals(33, pr.resultState.currentLocation.bitPos1b)
+ }
+ }
+ sptr.run()
+ }
+
+ //
+ // DFDL schema for delimited element.
+ //
+ private def delimitedSchema(term: String) = SchemaUtils.dfdlTestSchema(
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>,
+ <dfdl:format representation="text" ref="tns:GeneralFormat"/>,
+ <xs:element name="e1" type="xs:string" dfdl:lengthKind="delimited"
+ dfdl:terminator={ term } />)
+
+ /**
+ * Helper so we can test various delimiter-oriented scenarios.
+ *
+ * @param stringData Data to write. Should be small enough that the
parse will block.
+ * @param terminator String to use as terminating delimiter of
element. Can be more than one delimiter.
+ * @param followingDataString Data to write which should unblock the parse.
+ */
+ private def testHelperDaffodilParseDelimitedFromNetwork(
+ data: String,
+ terminator: String,
+ followingDataString: String) = {
+ val sptr = new SocketPairTestRig {
+ override def test(pos: OutputStream, cis: InputStream): Unit = {
+
+ val dp = TestUtils.compileSchema(delimitedSchema(terminator))
+
+ // Write the data. Should be too short to satisfy the parse.
+ //
+ pos.write(data.getBytes)
+ pos.flush()
+
+ val fut = Future {
+ TestUtils.runDataProcessorOnInputStream(dp, cis, areTracing = false)
+ }
+
+ Thread.sleep(100)
+ assertFalse(fut.isCompleted)
+ //
+ // Writing additional character(s) should unblock the parse.
+ //
+ pos.write(followingDataString.getBytes)
+ pos.flush()
+ val (pr, xml) = Await.result(fut, 100.milliseconds)
+ if (!pr.isError) {
+ assertEquals("1234", xml.text)
+ } else {
+ //parse failed.
+ val diagString = pr.getDiagnostics.map {
+ _.getMessage()
+ }.mkString("\n")
+ fail("Parse failed, but did not time-out.\n" + diagString)
+ }
+ }
+ }
+ sptr.run()
+ }
+
+ /**
+ * Test shows that for delimited data, we block seeking more data,
+ * but once the need for a terminator with longest match is satisfied
+ * the parse is unblocked.
+ */
+ // @Test // DAFFODIL-2504 - this fails looking for more data than is needed
for terminator.
+ def testDaffodilParseFromNetworkDelimited1(): Unit = {
+ intercept[TimeoutException] {
+ testHelperDaffodilParseDelimitedFromNetwork("1234", "$", "$")
+ }
+ fail("Test fails with a timeout exception")
+ }
+
+ /**
+ * This test works, and it should work. It characterizes that currently we
need to be able to read
+ * not only the terminator, but 7 more characters, in order for the reads
associated with the delimiter
+ * scanning to be satisfied.
+ *
+ *
+ */
+ @Test // DAFFODIL-2504
+ def testDaffodilParseFromNetworkDelimited1b(): Unit = {
+ testHelperDaffodilParseDelimitedFromNetwork("1234", "$",
+ "$1234567")
+ }
+
+ // @Test // DAFFODIL-2504 - this fails looking for more data than is needed
for terminator.
+ def testDaffodilParseFromNetworkDelimited2(): Unit = {
+ intercept[TimeoutException] {
+ testHelperDaffodilParseDelimitedFromNetwork("1234$", "$ $$", "$")
+ }
+ fail("Test fails with a timeout exception")
Review comment:
Same problem here. The fail() seems to imply the test should fail, and
the comment doesn't explain what is the problem (why is the test broken, why is
it being ignored?).
##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/util/TestUtils.scala
##########
@@ -22,15 +22,13 @@ import java.io.FileNotFoundException
import java.nio.channels.Channels
import java.nio.channels.ReadableByteChannel
import java.nio.channels.WritableByteChannel
-
import scala.util.Try
import scala.xml._
-
import org.apache.commons.io.output.NullOutputStream
-
import org.junit.Assert.assertEquals
+import org.apache.daffodil.Implicits._
-import org.apache.daffodil.Implicits._; object INoWarnU2 {
ImplicitsSuppressUnusedImportWarning() }
+import java.io.InputStream; object INoWarnU2 {
ImplicitsSuppressUnusedImportWarning() }
Review comment:
BTW, is there any relationship between the object INoWarnU2 and the
import it follows or precedes? If the object's standalone, why isn't it on a
line by itself?
##########
File path:
daffodil-core/src/test/scala/org/apache/daffodil/api/TestParseIndividualMessages.scala
##########
@@ -0,0 +1,230 @@
+/*
+ * 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.api
+
+import org.apache.daffodil.Implicits.intercept
+import org.apache.daffodil.io.SocketPairTestRig
+import org.apache.daffodil.util.SchemaUtils
+import org.apache.daffodil.util.TestUtils
+import org.junit.Assert._
+import org.junit.Test
+
+import java.io.InputStream
+import java.io.OutputStream
+import scala.concurrent.Await
+import scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent.Future
+import scala.concurrent.TimeoutException
+import scala.concurrent.duration.DurationInt
+import scala.xml.Node
+
+
+/**
+ * Shows that we can parse exactly 1 message from a TCP network socket
+ * without blocking for bytes past the end of the messsage.
+ *
+ * This only works for DFDL schemas of formats that are specified length.
+ */
+class TestParseIndividualMessages {
+
+ import SocketPairTestRig._
Review comment:
SocketPairTestRig is a nice addition, although what are you importing in
this line given that you've already imported SocketPairTestRig itself? If it's
only one method, I'd prefer to see it named explicitly (e.g., import
SocketPairTestRig.withTimeout).
##########
File path:
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala
##########
@@ -194,7 +194,7 @@ class DFDLTestSuite private[tdml] (
// That avoids creating the test suites repeatedly, but also leaks memory
unless
// you have an @AfterClass shutdown method in the object that calls
runner.reset() at end.
//
- // @deprecated("2016-12-30", "Use Runner(...) instead.")
+ // @deprecated("Use Runner(...) instead.", "2016-12-30")
Review comment:
Does the deprecation annotation work inside a comment?
##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/util/TestUtils.scala
##########
@@ -191,19 +189,33 @@ object TestUtils {
if (isError) {
throw new Exception(msgs)
}
- var p = saveAndReload(pf.onPath("/").asInstanceOf[DataProcessor])
+ val p = saveAndReload(pf.onPath("/").asInstanceOf[DataProcessor])
val pIsError = p.isError
if (pIsError) {
val msgs = pf.getDiagnostics.map(_.getMessage()).mkString("\n")
throw new Exception(msgs)
}
+ p
+ }
+
+ def runSchemaOnRBC(testSchema: Node, data: ReadableByteChannel, areTracing:
Boolean = false): (DFDL.ParseResult, Node) = {
+ runSchemaOnInputStream(testSchema, Channels.newInputStream(data),
areTracing)
+ }
+
+ def runSchemaOnInputStream(testSchema: Node, is: InputStream, areTracing:
Boolean = false): (DFDL.ParseResult, Node) = {
+ val p = compileSchema(testSchema)
+ runDataProcessorOnInputStream(p, is, areTracing)
+ }
+
+ def runDataProcessorOnInputStream(dp: DataProcessor, is: InputStream,
areTracing: Boolean = false): (DFDL.ParseResult, Node) = {
+ var p = dp
p = if (areTracing) {
- p.withDebugger(builtInTracer).withDebugging(true)
- } else p
+ p.withDebugger(builtInTracer).withDebugging(true)
+ } else p
Review comment:
Is this code indented correctly?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
