Skip to content

Add ignoreUnexpectedValidationErrors Attribute for test suite/cases#1301

Merged
olabusayoT merged 1 commit into
apache:mainfrom
olabusayoT:daf-2927-ignoreUnexpectedValidationErrors
Sep 19, 2024
Merged

Add ignoreUnexpectedValidationErrors Attribute for test suite/cases#1301
olabusayoT merged 1 commit into
apache:mainfrom
olabusayoT:daf-2927-ignoreUnexpectedValidationErrors

Conversation

@olabusayoT
Copy link
Copy Markdown
Contributor

  • remove validationError asserts and shouldValidate/expectValidationError variables
  • add tests

DAFFODIL-2927

@olabusayoT olabusayoT force-pushed the daf-2927-ignoreUnexpectedValidationErrors branch 2 times, most recently from 6e5843e to bf1b97f Compare September 17, 2024 16:39
Copy link
Copy Markdown
Member

@stevedlawrence stevedlawrence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 👍 minor suggestion on diagnosic, and wondering if we should deprecate or just remove the old syntax

optExpectedValidationErrors.isDefined
&& !optExpectedValidationErrors.get.exists(_.hasDiagnostics)
) { // <tdml:validationErrors/>
false // ignore unexpected validation errors will be false if <tdml:validationErrors />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth adding a System.err.println() that says something like

Use of <tdml:validationErrors /> is deprecated, use ignoreUnexpectedValidationError="false" instead.
Then eventually we can remove support for this.

Or maybe we should just remove support for it now? No tests in our regression suite use <tdml:validationErrors /> and I only see a handful of Daffodil tests use this that should be pretty straightforward to update.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up removing support for it, with a message about it being unsupported if <tdml:validationErrors/> is used

val msg = e.getMessage()
assertTrue(
msg.contains(
"ignoreUnexpectedDiags = false and test does not expect ValidationError diagnostics"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the error message so this says "ignoreUnexpectedValidationErrors = false instead of "ignoreUnexpectedDiags". ignoreUnexpectedDiags is kindof an internal flag that might confuse users since it doesn't map directly to what appears in tdml files.

Copy link
Copy Markdown
Contributor

@mbeckerle mbeckerle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@olabusayoT olabusayoT force-pushed the daf-2927-ignoreUnexpectedValidationErrors branch 3 times, most recently from 237f72d to 04372dc Compare September 19, 2024 18:24
- remove validationError asserts and shouldValidate/expectValidationError variables
- remove support for <tdml:validationErrors/>, it must have at least one child
- specify DiagnosticType specific message when there are unexpected diags
- add/update tests

DAFFODIL-2927
@olabusayoT olabusayoT force-pushed the daf-2927-ignoreUnexpectedValidationErrors branch from 04372dc to ecc0646 Compare September 19, 2024 18:54
@olabusayoT olabusayoT merged commit fc52160 into apache:main Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants