Add ability to specify errors that should not occur

[olabusayoT]

• using warnings/errors match attribute, specify whether all/any/none of the messages should match
• add verifyAnyOfDiagnosticsFound and verifyNoneOfDiagnosticsFound for any/none match attributes
• add tdml tests

DAFFODIL-864

[stevedlawrence]

It might be useful to reduce duplication by combining these functions into a single one that accepts a mode. It feels like are the core they are pretty similar just with some minor differences. For example, you could maybe do something like:

val diagsFound = expectedDiags.map { diag => 
  val wasFound = actualDiags.contains(diag)
  (diag, wasFound)
}

matchAttr match {
  case "all" if (!diagsFound.forall(_._2)) => {
     throw TDMLException(...)
  }
  case "any" if (!diagsFound.exists(_._2)) => {
     throw TDMLException(...)
  }
  case "none" if (diagsFound.exists(_._2)) => {
    throw TDMLException(...)
  }
}

So the logic to determine which diags are found is exactly the same with out copy/paste, and we create a Seq that determines which diags are found/not found. Then a match/case does the logic to figure out which of those were failures depending on the match mode and to create the appropriate exception.

[mbeckerle]

Needs a way to specify no warnings from a test or a test suite.

Needs to cover errors (match all and match none) as well as warnings (match all and match none). I don't think match 'any' is needed.

[mbeckerle]

I'm not seeing logic here about warnings i.e., how we control whether warnings are tolerated at all.

[mbeckerle]

+1

A separate task once this is committed is to go through the Daffodil test suites and convert them all to default that no-warnings are allowed, adding expected warnings, or configs with warning suppression, to each test that needs it.

We should avoid adding lots of expected warnings to tests where the warning is not related to the test. It's better to suppress those warnings via a tweak to the test schema, or adding a suppression config for that class of warning in that case.

I think this will end up being a fairly big job however, and I'm guessing done incrementally, not all at once.

[stevedlawrence]

Looks good, especially since TDML runner has really grown very complex, handful of questions about how things work that are probably worth a discussion since there are clearly many different ways expected results can be interpreted.

[stevedlawrence]

If I'm understanding this e.nonEmpty check, the idea is that it will ignore empty <tdml:errors /> elements? Is that what we want? I'm not sure exactly what the right thing should be, but it feels wrong to ignore it.

My intuition says <tdml:errors /> means this test should fail, but I don't care what the diagnostics are. And similarly that would mean <tdml:warnings /> would mean there should be warnings, but I don't care what they are. But this is also confusing because <tdml:validationErrors /> means there should be no validation errors.

I'm not sure what the right behavior is, but we probably need to make sure everything is consistent.

I'm also fine saying an empty tdml:errors or tdml:warnings is invalid, but that also isn't consistent with an empty tdml:validationErrors. Maybe empty tdml:validationErrors is does the opposite of what it should? And we really need a noValidationErrors attribute as well?

[olabusayoT]

I'm of the mind to have them all behave consistently. So if we have <tdml:validationErrors /> mean that there should be no validation errors, it should be the same for <tdml:errors /> and <tdml:warnings />. But if we have that functionality, then I don't think we need the noWarnings/noValidationErrors attributes. Thoughts?

[stevedlawrence]

The problem if we didn't support noWarnings is it would be a huge effort to add <tdml:warnings /> to all tests to ensure warnings are never ignored. We eventually want get to the point where no tests have unexpected warnings and your new property is definitely the preferred way to enforce that globally over <tdml:warnings /> which can't be added globally.

If we do want consistency with validation, we may want to consider deprecating <tdml:validation /> and replacing it with a property similar to the noWarnings one . Just like with warnings, we probably want to get the the point where there are no unexpected validation errors, and it's much easier to enforce that by flipping a default flag than adding <tdml:validaiton /> to all the tests. If we do this, I'd suggest it be a separate PR since there's backwards compatibility issues to consider.

Also, I don't think it's worth the added complexity right now to support something like <tdml:errors /> means "some errors exist but I don't care about the message". That might be useful at some point, but that could also be a separate PR if we want it. The key thing we really need is the noWarnings capability.

[stevedlawrence]

Some more thoughts. I would suggest we make it so <tdml:warnings /> and <tdml:errors /> is not allowed--each one must have child elements. That keeps things simple an intuitive. We don't really need a way to say "there must be an error/warning, but I don't care what it is"--that just isn't really that necessary and adds complication.

For now, we still allow <tdml:validation /> for backwards compatibility, but we should eventually plan to deprecate that in favor of a new property similar to the "no warnings" property. And when we add that property, we can support backwards compatibility by making it so <tdml:validationErrors /> enables the "noValidationErrors" flag, but is otherwise ignored. Eventually we can drop that backwards compatibility and make <tdml:validationErrors /> invalid to match <tdml:warnings /> and <tdml:errors />`. These changes to how validationErrors works can all be done in future PRs though. It's fine if there is a slight inconsistency for now.

[olabusayoT]

Sounds good

[stevedlawrence]

Should this also be in a isCrossTest check like errors an warnings?

[stevedlawrence]

I think these checks are going to make it a bit of a pain to globally change the noWarnings default to true (which we probably want to do at some point) since it means every test that wants to check for warnings would have to add noWarnings = false. A quick grep shows about 50 test that look for it, so it's not a huge numbe, but still a pain to remember to have to add it.

It feels the existence of <tdml:warnings> should imply you want to check warnings and so noWarnings should be ignore? Which might mean that the noWarnings flag really wants to define what to do when there are no expected warnings (i.e. there is no <tdml:warnings> element) but warnings exist. The current behavior is to just ignore warnings, but I think we want the ability to change that behavior to an error. That way if <tdml:warnings> do exist, then we ignore that flag, and we make sure that the new none/all logic is used.

And I think it would just need a small change to the logic you have, something like this I think?

if (optWarnings.nonEmpty) {
  VerifyTestCase.verifyDiagnosticsFound(..., optWarnings, ...)
} else if (warningDiagnostics.nonEmpty && noWarnings) {
  throw TDMLException("test does not expect warnings, but warnings were found and noWarnings == true means that's an error")
} else {
  // test does not expect warnings, and either there were no warnings or the flag says we
  // should ignore them if they exist, so this is a no-op
}

We might want a different name for noWarnings since I don't think it makes that behavior clear? I can't think of a clear name that isn't super verbose, but I'm some we could come up with something reasonable.

[olabusayoT]

I updated the logic and renamed noWarnings to noWarningsAllowed

[stevedlawrence]

If I'm interpreting the case right, it is for a test that has both an expected infoset and expected errors with match= "none". Feels like a tests should never need to do that. If you expect an infoset then either you don't get an infoset and the test fails regardless of diagnostics, or you do get an infoset which means there are no diagnostics to even check against. So I would suggest this case shouldn't be needed and if a test does exist then we should drop to the below match case and create a TDML exception saying the test case is invalid.

Feels to me like if there is tdml:errors with match = "none" the interpretation is that we never expect an infoset an we always expect some error diagnostics to exist, we just expect certain strings do not appear in those diagnostics.

[stevedlawrence]

I think we should still have this assertion at all times. If we expect validationErrors (which this case is about) then validation errors must exist.

I think this means a check in verifyDiagnosticsFound isn't working correct? Maybe the issue that match="none" doesn't require validation errors? I would suggset that match="none" means that at least one validation error must exist, just that certain strings just should not appear in the diagnostics.

[stevedlawrence]

Fix indentation.

[stevedlawrence]

I thin this last condition is incorrect? match="none" means we still expect to see error/warning/validation (whatever type we are expecting), we just don't want to see certain strings in the diagnostics we find.

[stevedlawrence]

I might suggest moving the condition of each case into the actual case, that way you don't need the case _ at the end, or it can be changed ot an Asssert.impossible. So if we ever add a new "match" option we won't accidentally fall into it.

Also, since we throw an exception on the first expected diag that isn't found, it might be cleaner to use find + get instead of filterNot +map. For example:

case "all" => {
  if (!diagsFound.forall(_.2)) {
    throw new TDMLExcption(
    "Did not find ... message " +
    diagsFound.find(_._2 == false).get._1 +
    " in any of the actual ... " + 
    ...
  )
}

Or alternatively, you could even avoid the forall and just do:

case "all" => {
  diagsFound.find(_._2 == false).foreach { case (d, _) =>
    throw new TDMLExcption(
      "Did not find ... message " +
      d +
      " in any of the actual ... " + 
      ...
    )
  }
}

And the none case would be the same just _._2 == true. The first one is a little less efficient, but it feels a bit cleaner to me and more clear since the forAll/exists mor clearly matches "all" and "none". But I think both are maybe a little more clear over a filter + map.

[stevedlawrence]

This is unexpected, any idea what cause this change in message?

[olabusayoT]

I looked into it and I couldn't find nonNegA in the schema root4b at all. I'm not sure how it passed all this time

[stevedlawrence]

I just noticed that this test expects <tdml:errors>. Does it even make sense to expect validation errors if we don't even get an infoset? I wonder if this was just previously ignored since the tests didn't expect success? Should the expected validationErrors just be removed here, and in general tdml:validationErrors + tdml:errors in the same test should maybe be an TDMLException? We can open a bug for that if that makes sense.

[olabusayoT]

Good point, I did just add validationErrors to checkDiagnostics, which explains why the test just now failed.

I think it makes sense to make validationErrors errors in the same test a TDMLException in a future PR

[olabusayoT]

For the time being, I'll remeove the expected validation error

[stevedlawrence]

Looks good, just and handful of thoughts for more possible cleanup so this complex test runner isn't so complex.

[stevedlawrence]

Update comment to reference new name and meaning.

[stevedlawrence]

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 ` elements. Something like

val diagnoticNodes = {
  val nodes = node \\ "error"
  if (nodes.isEmpty) throw TDMLExceptino(...)
  nodes
}

And then this function just becomes

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.

[stevedlawrence]

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?

[olabusayoT]

Yes, making that change will allow us to call checkDiagnosticMessages everywhere we currently call verifyDiagnosticsFound. We would just pass in None for what we don't wish to check.

Ex to check only warnings or only validationErrors, we do

        checkDiagnosticMessages(
          allDiags,
          None,
          optWarnings,
          None,
          implString
        )
    //or 
        checkDiagnosticMessages(
          actual.getDiagnostics,
          None,
          None,
          optValidationErrors,
          implString
        )
    

[stevedlawrence]

Seems reasonable. But are there cases where we don't what to check all expected diagnostics? Seems like during each test we do a parse/unparse/whatever and then check that all actual diagnostics match the expected diagnostics.

[olabusayoT]

I think so, in verifyParseResult for example, it looks like we check validation errors separate from warnings

(shouldValidate, expectsValidationError) match {
      case (_, true) => {
        // Note that even if shouldValidate is false, we still need to check
        // for validation diagnostics because failed assertions with
        // failureType="recoverableError" are treated as validation errors
        checkDiagnosticMessages(
          actual.getDiagnostics,
          None,
          None,
          optExpectedValidationErrors,
          implString
        )
        Assert.invariant(actual.isValidationError)
      }
      case (true, false) => {
        VerifyTestCase.verifyNoValidationErrorsFound(
          actual,
          implString
        ) // Verify no validation errors from parser
        Assert.invariant(!actual.isValidationError)
      }
      case (false, false) => // Nothing to do here.
    }

    val allDiags = compileWarnings ++ actual.getDiagnostics
    checkDiagnosticMessages(
      allDiags,
      None,
      optExpectedWarnings,
      None,
      implString
    )

[stevedlawrence]

I imagine some of that can be refactored and simplified now that you've cleaned up much of this and made things more generic and combined functionality. Seems all three of those checks can be done with one call to checkDiagnosticMessages.

I guess we do still need None sometimes, since some functions (e.g. runParseExpectSuccess) don't have access to certain diagnostics because they expect success and really do need to pass in None. I think maybe the separate expectSuccess/expectFailure funtions are complicating things a bit, but that clean up can be done for another day.

[stevedlawrence]

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:

val hasUnexpectedDiags = actualDiagsFiltered.isNonEmpty && expectedDiags.isEmpty
if (hasUnexpectedDiags && !ignoreUnexpectedDiags)
  throw new TDMLExecption(found unexpected diags)

[olabusayoT]

actualDiagsFiltered will be empty if expectedDiags.isEmpty...since it uses the type of the expectDiags for filtering, but I think we can just change that to actualDiags and the logic works.

Unless we want actualDiagsFiltered to return unfiltered actualDiags when we have no expected diags, thoughts on that?

[stevedlawrence]

If we use actualDiags then I think it will error if we have any diagnostics, not just unexpected diagnostics of the right type. Feels like we should pass in an an enum that says which diag we are checking, or pass in a filter function that is passed in that is used to pick wich diagnostics to check. Then we don't have to rely on the expected diags to know which actual diags to compare against.

[olabusayoT]

Hmm you're right...ok, will add enum called diagnosticType

[stevedlawrence]

Another alternative, if you update everything to calls checkDiagnostisMesssages you could do the filter in there, since each call to verifyDiagnosticFound knows which type it's verifying.

[stevedlawrence]

Can this just return a Unit? I think this function either throws an exception if there are problems or returns nothing?

[stevedlawrence]

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.

[stevedlawrence]

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.

[stevedlawrence]

+1 looks great, this is a big improvement and I think will help alot to make the test runner less complex.

Just one more place where maybe something might not be needed anymore, but I'm not positive.

[stevedlawrence]

Do we need this case anymore? You've updated checkDiagnosicts to check validation errors, including whether or not to ignore unexpected errors. I think this match case goes away completely and we just use the default case? And verifyNoValidatioNErrorsFound goes away?

[stevedlawrence]

Same comment as above? Does this match case go away?