mbeckerle commented on a change in pull request #233: Warn when DFDL properties
are ignored
URL: https://github.com/apache/incubator-daffodil/pull/233#discussion_r293903681
##########
File path:
daffodil-test/src/test/resources/org/apache/daffodil/section13/text_number_props/TextNumberProps.tdml
##########
@@ -3462,10 +3467,10 @@
<xs:element name="nanInvalid" dfdl:lengthKind="delimited"
dfdl:terminator="]" type="xsd:double"
dfdl:textStandardNaNRep="%#r" />
- <xs:element name="infInvalidType" dfdl:lengthKind="delimited"
type="xsd:long" dfdl:textStandardInfinityRep="INFINITY"
+ <xs:element name="infInvalidType" dfdl:lengthKind="delimited"
type="xsd:long"
dfdl:terminator="]" />
- <xs:element name="nanInvalidType" dfdl:lengthKind="delimited"
type="xsd:long" dfdl:textStandardNaNRep="NOTANUMBER"
+ <xs:element name="nanInvalidType" dfdl:lengthKind="delimited"
type="xsd:long"
Review comment:
This looks like a test bug. I.e., the test should have been written to use a
xs:float or xs:double so that the property would be relevant. The test just
looks for a parse error complaining about the NOTANUMBER syntax. But if you
remove this property, it's not really anything about nan anymore, and so the
element name isn't right, nor is the name of the associated test. It's really
just a test that non-numeric data won't parse as a long.
I feel like this test should have been changed into one that is looking for
the new warning about dfdl:textStandardNaNRep being ignored on an xsd:long.
That's probably covered elsewhere, so arguably, this test here is just plain
not relevant anymore, and given the misnomers, should probably just be deleted.
To what degree have tests now evolved into "poorly named tests" that don't
really test the things they were originally testing? Is the quality of the
tests declining/being-diluted now? Arguably, introduction of this new uniform
detection capability for unused properties obviates quite a few tests that were
introducting such properties in some sense to test that they were in fact being
ignored.
Do we have a backlog issue now of re-scrutinizing tests to be sure they're
still doing meaningful testing?
This is really just a discussion topic that could be held on dev@ mailing
list and isn't really core to this review.
----------------------------------------------------------------
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]
With regards,
Apache Git Services
