stevedlawrence commented on code in PR #1204:
URL: https://github.com/apache/daffodil/pull/1204#discussion_r1756898964
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -634,15 +642,32 @@ abstract class TestCase(testCaseXML: NodeSeq, val parent:
DFDLTestSuite) {
lazy val optExpectedOrInputInfoset = (testCaseXML \
"infoset").headOption.map { node =>
new Infoset(node, this)
}
- lazy val optExpectedErrors: Option[ExpectedErrors] = (testCaseXML \
"errors").headOption.map {
- node => ExpectedErrors(node, this)
+ lazy val optExpectedErrors: Option[Seq[ExpectedErrors]] = {
+ val errors = testCaseXML \ "errors"
+ val error = (errors \\ "error")
Review Comment:
Thoughts on moving the `<tdml:errors />` check to the `ExpectedErrors class?
It simplifies this a bit and means the ExpectedErrors class contains the logic
about what makes an ExpectedError valid or not, which is probably where that
logic belongs, especially since it alreadys is looking a the `<error>`
elements. Something like
```scala
val diagnoticNodes = {
val nodes = node \\ "error"
if (nodes.isEmpty) throw TDMLExceptino(...)
nodes
}
```
And then this function just becomes
```scala
val errors = testCaseXML \ "errors"
val s = errors.map { node => ExpectedErrors(node, this) }
if (s.nonEmpty) Some(s) else None
```
Same for expected warnings.
This also makes it consitent with the ExpectedValidationErrors change. So
the only differences are withing the ExpectedErrors/Warning classes.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -1806,68 +1881,101 @@ object VerifyTestCase {
* @param expectedDiags Expected diagnostics
from test cases
* @param implString Implementation string
*/
- def verifyAllDiagnosticsFound(
+ def verifyDiagnosticsFound(
actualDiags: Seq[Diagnostic],
- expectedDiags: Option[ErrorWarningBase],
+ expectedDiags: Option[Seq[ErrorWarningBase]],
+ ignoreUnexpectedWarnings: Boolean,
Review Comment:
Can this parameter be changed to a more generic `ignoreUnexpectedDiags:
Boolean`? For example, when the `expectedDiags` are `ExpectedErrors` we would
always pass in false (we never ignore unexpected errors). When `expectedDiags`
are `ExpectedWarnings` we would pass in the value of
`ignoreUnexpectedWarnings`. That way this function doesn't need to have have
specific logic for warnings (or validationErrors when we add
ignoreUnexpectedValidationErrors). It just has something like:
```scala
val hasUnexpectedDiags = actualDiagsFiltered.isNonEmpty &&
expectedDiags.isEmpty
if (hasUnexpectedDiags && !ignoreUnexpectedDiags)
throw new TDMLExecption(found unexpected diags)
```
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -1806,68 +1881,101 @@ object VerifyTestCase {
* @param expectedDiags Expected diagnostics
from test cases
* @param implString Implementation string
*/
- def verifyAllDiagnosticsFound(
+ def verifyDiagnosticsFound(
actualDiags: Seq[Diagnostic],
- expectedDiags: Option[ErrorWarningBase],
+ expectedDiags: Option[Seq[ErrorWarningBase]],
+ ignoreUnexpectedWarnings: Boolean,
implString: Option[String]
- ) = {
+ ): Option[Seq[Unit]] = {
- val actualDiagMsgs = actualDiags.map {
- _.toString()
- }
val expectedDiagMsgs = expectedDiags
- .map {
- _.messages
+ .map { d =>
+ d.flatMap(_.messages)
}
.getOrElse(Nil)
- if (expectedDiags.isDefined && actualDiags.isEmpty) {
+ val expectedDiagnosticType = if (expectedDiags.isDefined) {
+ expectedDiags.get.head.diagnosticType
+ } else {
+ ""
+ }
+ val actualDiagsFiltered = if (expectedDiagnosticType == "validation
error") {
+ actualDiags.filter(diag => diag.isValidation)
+ } else if (expectedDiagnosticType == "warning") {
+ actualDiags.filterNot(d => d.isValidation || d.isError)
+ } else if (expectedDiagnosticType == "error") {
+ actualDiags.filter(_.isError)
+ } else {
+ Seq.empty
+ }
+
+ val actualDiagsFilteredMessages = actualDiagsFiltered.map(_.toString())
+
+ // throw exception if we have no actualDiags, but have expected diagnostics
+ if (
+ expectedDiags.isDefined
+ && actualDiagsFiltered.isEmpty
+ ) {
throw TDMLException(
""""Diagnostic message(s) were expected but not found."""" +
"\n" +
"""Expected: """ + expectedDiagMsgs.mkString("\n") +
- (if (actualDiagMsgs.isEmpty)
- "\n No diagnostic messages were issued."
+ (if (actualDiagsFiltered.isEmpty)
+ s"\n No ${expectedDiagnosticType} diagnostic messages were
issued."
else
- "\n The actual diagnostics messages were: " +
actualDiagMsgs.mkString("\n")),
+ "\n The actual diagnostics messages were: " +
actualDiagsFilteredMessages.mkString(
+ "\n"
+ )),
implString
)
}
- // must find each expected warning message within some actual warning
message.
- expectedDiags.foreach { expectedDiag =>
- {
- expectedDiag.messages.foreach { expectedMsg =>
- {
- val wasFound = actualDiags.exists { actualDiag =>
- val actualDiagMsg = actualDiag.getMessage()
- actualDiag.isError == expectedDiag.isError &&
- actualDiagMsg.toLowerCase.contains(expectedMsg.toLowerCase)
- }
- if (!wasFound) {
- throw TDMLException(
- s"""Did not find diagnostic ${expectedDiag.diagnosticType}
message """" +
- expectedMsg +
- """" in any of the actual diagnostic messages: """ + "\n" +
- actualDiagMsgs.mkString("\n"),
- implString
- )
- }
+ if (
+ !actualDiags.forall(d => d.isValidation || d.isError)
+ && expectedDiags.isEmpty
+ && !ignoreUnexpectedWarnings
+ ) {
+ throw TDMLException(
+ s"ignoreUnexpectedWarnings = false and test does not expect warnings,
but created the following: " +
+ s"${actualDiags.filterNot(d => d.isValidation ||
d.isError).mkString("\n")}",
+ implString
+ )
+ }
Review Comment:
Yeah, here it might be nice to make this a more generic check.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -1806,68 +1881,101 @@ object VerifyTestCase {
* @param expectedDiags Expected diagnostics
from test cases
* @param implString Implementation string
*/
- def verifyAllDiagnosticsFound(
+ def verifyDiagnosticsFound(
actualDiags: Seq[Diagnostic],
- expectedDiags: Option[ErrorWarningBase],
+ expectedDiags: Option[Seq[ErrorWarningBase]],
+ ignoreUnexpectedWarnings: Boolean,
implString: Option[String]
- ) = {
+ ): Option[Seq[Unit]] = {
- val actualDiagMsgs = actualDiags.map {
- _.toString()
- }
val expectedDiagMsgs = expectedDiags
- .map {
- _.messages
+ .map { d =>
+ d.flatMap(_.messages)
}
.getOrElse(Nil)
- if (expectedDiags.isDefined && actualDiags.isEmpty) {
+ val expectedDiagnosticType = if (expectedDiags.isDefined) {
+ expectedDiags.get.head.diagnosticType
+ } else {
+ ""
+ }
+ val actualDiagsFiltered = if (expectedDiagnosticType == "validation
error") {
+ actualDiags.filter(diag => diag.isValidation)
+ } else if (expectedDiagnosticType == "warning") {
+ actualDiags.filterNot(d => d.isValidation || d.isError)
+ } else if (expectedDiagnosticType == "error") {
+ actualDiags.filter(_.isError)
+ } else {
+ Seq.empty
+ }
+
+ val actualDiagsFilteredMessages = actualDiagsFiltered.map(_.toString())
+
+ // throw exception if we have no actualDiags, but have expected diagnostics
+ if (
+ expectedDiags.isDefined
+ && actualDiagsFiltered.isEmpty
+ ) {
throw TDMLException(
""""Diagnostic message(s) were expected but not found."""" +
"\n" +
"""Expected: """ + expectedDiagMsgs.mkString("\n") +
- (if (actualDiagMsgs.isEmpty)
- "\n No diagnostic messages were issued."
+ (if (actualDiagsFiltered.isEmpty)
+ s"\n No ${expectedDiagnosticType} diagnostic messages were
issued."
else
- "\n The actual diagnostics messages were: " +
actualDiagMsgs.mkString("\n")),
+ "\n The actual diagnostics messages were: " +
actualDiagsFilteredMessages.mkString(
+ "\n"
+ )),
implString
)
}
- // must find each expected warning message within some actual warning
message.
- expectedDiags.foreach { expectedDiag =>
- {
- expectedDiag.messages.foreach { expectedMsg =>
- {
- val wasFound = actualDiags.exists { actualDiag =>
- val actualDiagMsg = actualDiag.getMessage()
- actualDiag.isError == expectedDiag.isError &&
- actualDiagMsg.toLowerCase.contains(expectedMsg.toLowerCase)
- }
- if (!wasFound) {
- throw TDMLException(
- s"""Did not find diagnostic ${expectedDiag.diagnosticType}
message """" +
- expectedMsg +
- """" in any of the actual diagnostic messages: """ + "\n" +
- actualDiagMsgs.mkString("\n"),
- implString
- )
- }
+ if (
+ !actualDiags.forall(d => d.isValidation || d.isError)
+ && expectedDiags.isEmpty
+ && !ignoreUnexpectedWarnings
+ ) {
+ throw TDMLException(
+ s"ignoreUnexpectedWarnings = false and test does not expect warnings,
but created the following: " +
+ s"${actualDiags.filterNot(d => d.isValidation ||
d.isError).mkString("\n")}",
+ implString
+ )
+ }
+
+ expectedDiags.map { diags =>
+ diags.map { diag =>
+ val matchAttr = diag.matchAttrib
+ val diagsFound = diag.messages.map { d =>
+ // make case insensitive
+ val wasFound =
+ actualDiagsFilteredMessages.exists(ad =>
ad.toLowerCase.contains(d.toLowerCase))
+ (d, wasFound)
+ }
+ matchAttr match {
+ case "all" if (!diagsFound.forall(_._2)) => {
Review Comment:
Technically, you probably want the condition inside the case block. For
example, currently if matchAttr is "all" but the condition fails then it will
do the "none" and efault cases, which shouldn't happen. And that way, the
default case implies the user had something like `match="foo"` and it should be
an error.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -169,6 +169,8 @@ private[tdml] object DFDLTestSuite {
*
* shouldDoWarningComparisonOnCrossTests controls whether test warning
messages are compared
* during cross testing.
+ *
+ * defaultNoWarningsDefault specifies that a test should not create warnings
at all
Review Comment:
Update comment to reference new name and meaning.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -959,32 +990,45 @@ abstract class TestCase(testCaseXML: NodeSeq, val parent:
DFDLTestSuite) {
protected def checkDiagnosticMessages(
diagnostics: Seq[Diagnostic],
- errors: ExpectedErrors,
- optWarnings: Option[ExpectedWarnings],
+ errors: Seq[ExpectedErrors],
Review Comment:
Is there value in making `errors` an `Option[Seq[ExpectedErrors]]` and then
checkDiagnosticMessages can be use anywhere diagnostics need to be checked, not
just for the case where errors are expected? The TDML Runner has really gotten
complex over time, so having one way to check diagnostics might help to
simplify things. You went down the path of making checkDiagnostics also do
validation checks so it could be used in more places. Would making errors
optional make it allowed to be used everywhere?
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -1806,68 +1881,101 @@ object VerifyTestCase {
* @param expectedDiags Expected diagnostics
from test cases
* @param implString Implementation string
*/
- def verifyAllDiagnosticsFound(
+ def verifyDiagnosticsFound(
actualDiags: Seq[Diagnostic],
- expectedDiags: Option[ErrorWarningBase],
+ expectedDiags: Option[Seq[ErrorWarningBase]],
+ ignoreUnexpectedWarnings: Boolean,
implString: Option[String]
- ) = {
+ ): Option[Seq[Unit]] = {
Review Comment:
Can this just return a `Unit`? I think this function either throws an
exception if there are problems or returns nothing?
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -1806,68 +1881,101 @@ object VerifyTestCase {
* @param expectedDiags Expected diagnostics
from test cases
* @param implString Implementation string
*/
- def verifyAllDiagnosticsFound(
+ def verifyDiagnosticsFound(
actualDiags: Seq[Diagnostic],
- expectedDiags: Option[ErrorWarningBase],
+ expectedDiags: Option[Seq[ErrorWarningBase]],
+ ignoreUnexpectedWarnings: Boolean,
implString: Option[String]
- ) = {
+ ): Option[Seq[Unit]] = {
- val actualDiagMsgs = actualDiags.map {
- _.toString()
- }
val expectedDiagMsgs = expectedDiags
- .map {
- _.messages
+ .map { d =>
+ d.flatMap(_.messages)
}
.getOrElse(Nil)
- if (expectedDiags.isDefined && actualDiags.isEmpty) {
+ val expectedDiagnosticType = if (expectedDiags.isDefined) {
+ expectedDiags.get.head.diagnosticType
+ } else {
+ ""
+ }
+ val actualDiagsFiltered = if (expectedDiagnosticType == "validation
error") {
+ actualDiags.filter(diag => diag.isValidation)
+ } else if (expectedDiagnosticType == "warning") {
+ actualDiags.filterNot(d => d.isValidation || d.isError)
+ } else if (expectedDiagnosticType == "error") {
+ actualDiags.filter(_.isError)
+ } else {
+ Seq.empty
+ }
Review Comment:
I general prefer to avoid comparing strings, they tend to be fragile. For
example, if we one day decide to capitalize these this stuff will break.
Instead, we can use Scala type system, e.g.
```
val expectedDiagType = expectedDiags.headOption
val actualDiagsFiltered = expectedDiagType match {
case Some(_: ExpectedValidationError) => actualDiags.filter(diag =>
diag.isValidation)
case Some(_: ExpectedWarning) => actualDiags.filterNot(d => d.isValidation
|| d.isError)
case Some(_: ExpectedError) => actualDiags.filter(_.isError)
case None => Seq.empty
}
```
That said, it might be useful to pass in an enum or a filter function or
something as a paramter, or do the filtering in checkDiagnosticMessages
(assuming that can be updated so everyone calls it). Something like that might
be preferred, since this ends up with `Seq.empty` if we don't know the type,
which makes it look like there are no actual diagnostics to check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
