Corrected a spurious warning about the "value" attribute of VOTable <option>.

[gpdf]

The table_test.py tests on gemini.xml exposed this bug but the "correct"
return value from several tests was incorrectly set to accept the spurious
warnings as acceptable.

There are still 23 warnings on the existing test file; at some point it
may be reasonable to look at the file by hand and determine whether it
can be made more conformant, and then carry out the tests in future
both on the conformant XML and the existing version, so as to have one
test that expects no warnings, and another that tests that warnings are
properly issued for bad XML.

[pllim]

Hi. Can you please elaborate on this bug that you found? What did you see vs what you expected to see? Can you also add a regression test for this particular bug so it does not get re-introduced?

p.s. Don't worry about the change log for now, as we're in the middle of feature freeze.

[gpdf]

Sorry, too late for the change log.

The error was a one-word mistake in tree.py that had "data" and "name" as the acceptable attributes of the VOTable element; this is incorrect and the acceptable attributes are "value" and "name".

Two existing tests in io/votable/tests produced this spurious message, but unfortunately the test assertions were written to consider it acceptable. For vo_test.py the spurious error,

W48: Unknown attribute 'value' on OPTION <OPTION name="bogus" value="whatever"/>

was included in the associated validation.txt file, while for table_test.py the spurious errors were accounted for in several tests that were set up to accept exactly 25 parse warnings, of which two were the spurious ones.

There is no need to add additional tests, I think, because this code path is already being tests and my corrections to the test cases would catch a future regression.

[pllim]

I won't have time to dig at the VO standards to confirm this before 4.0 feature freeze, so I tentatively milestoned it for 4.0.1, unless @tomdonaldson has time... 😬

[pllim]

This needs to go in the "bug fix" sub-section. But please hold off on moving it until we can be sure of the milestone. Thanks!

[gpdf]

No problem. Thanks for being patient.

[gpdf]

@pllim I don't know whether @tomdonaldson will have time, but I posted some pre-digested snippets from the VOTable standard in a comment last night to make that review easier.

[pllim]

Thank you, that'll certainly help!

[gpdf]

As I suggested in one of the commit messages, it would be a further improvement to the test suite if someone - I could perhaps do this on another occasion - hand-cleaned the gemini.xml file to remove, if possible, all of the current 23 warnings (most of which appear justified to me), saving a new version, gemini_clean.xml, and adding at least one test based on it. In that case you would have a test with 0 expected warnings, which might make future maintainers less willing to dismiss warnings as unavoidable breakage (which may be how the present bug was overlooked). But it would still be useful to keep the existing gemini.xml file as well, so that the alternate code paths for the warnings are actually tested.

[gpdf]

Here are the references that are relevant:

VOTable 1.3, section 4.7, narrative text in paragraph beginning "All three MIN, MAX and OPTION sub-elements" confirms that <OPTION> has "value" and "name" attributes.
http://www.ivoa.net/documents/VOTable/20130920/REC-VOTable-1.3-20130920.html#ToC31
(direct link to text about <OPTION>):
http://www.ivoa.net/documents/VOTable/20130920/REC-VOTable-1.3-20130920.html#elem:OPTION

VOTable 1.3, section 7.2, Attribute Summary Table, confirms this schema:
http://www.ivoa.net/documents/VOTable/20130920/REC-VOTable-1.3-20130920.html#ToC46

XSD for <OPTION>, from VOTable 1.3, Appendix B:
http://www.ivoa.net/documents/VOTable/20130920/REC-VOTable-1.3-20130920.html#ToC62

<xs:complexType name="Option">
<xs:sequence>
<xs:element name="OPTION" type="Option" minOccurs="0" maxOccurs="unbounded"/>
</xs:sequence>
<xs:attribute name="name" type="xs:token"/>
<xs:attribute name="value" type="xs:string" use="required"/>
</xs:complexType>

The same references in VOTable 1.4:
http://www.ivoa.net/documents/VOTable/20191021/REC-VOTable-1.4-20191021.html#ToC32
http://www.ivoa.net/documents/VOTable/20191021/REC-VOTable-1.4-20191021.html#ToC47
http://www.ivoa.net/documents/VOTable/20191021/REC-VOTable-1.4-20191021.html#elem:OPTION
http://www.ivoa.net/documents/VOTable/20191021/REC-VOTable-1.4-20191021.html#ToC64

In addition, I think examining the source code directly will confirm that the erroneous presence of the string "data" was a very plausible cut/paste error or "think-o" arising from the frequent use of "data" as a variable name in that paragraph of code.

[gpdf]

I noticed that one of the automated tests showed a loss of test coverage from this PR. This is because it turns out that no legitimate instance of the "W48 Unknown attribute" error had been included in the test files. Thus, after the bug fix, the message was no longer emitted and the code path to emit it was not tested. Solution: add an actual instance of this fault to the gemini.xml test input and adjust the test expectations accordingly; I hope this restores the lost test coverage.

[gpdf]

I see the "continuous-integration/travis-ci/pr — The Travis CI build failed" and I have looked at the log file for the failing step at https://travis-ci.org/astropy/astropy/jobs/602689700?utm_medium=notification&utm_source=github_status

I don't see anything actionable for me in that log - there are no signs of failure that I've been able to discern up through the moment that the post-build test fails with

from astropy import version as astropy_version

ModuleNotFoundError: No module named 'astropy'

Please let me know if there's anything I can do.

[bsipocz]

@gpdf - That is unrelated, we did some refactoring for the pytest plugins we use. I'm not exactly certain how that ended up in that travis job, restarted it now, it should pass now. (but if it still has issues, we'll deal with that separately).

[tomdonaldson]

I agree with the assessment of this problem. The spec calls for the OPTION element to allow 'value' and 'name' elements, and the code does the same thing except in this one case where it's checking whether to issue the warning. This PR corrects that warning check, and I like that the gemini.xml test case was modified to exercise the warning code.

[bsipocz]

Given the approval above, I've moved the changelog to the bugfix section, and merging this now.

Thank you @gpdf!