Skip to content

TST: Get rid of io.votable test warnings#8756

Merged
pllim merged 1 commit intoastropy:masterfrom
pllim:tests-vo-warn-2
Oct 3, 2019
Merged

TST: Get rid of io.votable test warnings#8756
pllim merged 1 commit intoastropy:masterfrom
pllim:tests-vo-warn-2

Conversation

@pllim
Copy link
Member

@pllim pllim commented May 24, 2019

This should get rid of the rest of io.votable test warnings. Follow up of #8715 and as part of work for #7928 . The warnings were captured by removing the line in setup.cfg to suppress pytest warnings and then add this in the same section:

[pytest]
filterwarnings =
    error

And then run tests with python setup.py -P io.votable. Most of the changes are to handle VOWarning emitted when writing VO table back out or parsing specific fields.

@codecov

This comment has been minimized.

None, name='c', datatype='char',
config=config)
with pytest.warns(exceptions.W47):
field = tree.Field(None, name='c', datatype='char', config=config)
Copy link
Member

Choose a reason for hiding this comment

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

Just a general comment - I think it would be nice to make sure in each case that we know why the warning is being raised. Currently if I look at this test it's not clear why this specific line of code should raise say W47, and we need to make sure that it is correct that the warning is being emitted. Why does it warn and not raise an error given the config? Is that expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! 😱

Copy link
Member Author

Choose a reason for hiding this comment

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

For this particular line, it is from W47: ?:?:?: W47: Missing arraysize indicates length 1:

...\io\votable\converters.py in __init__(self, field, config, pos)
    300
    301         if field.arraysize is None:
--> 302             vo_warn(W47, (), config, pos)
    303             field.arraysize = '1'

So, this one is okay to ignore. As for the others, sigh, I need to find another day to Sherlock through the warnings...

This comment was marked as outdated.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the deeper design question on how much config should control how all the warnings are handled, I have opened a separate issue at #8775

field = tree.Field(
None, name='c', datatype='char',
config=config)
field = tree.Field(None, name='c', arraysize='1', datatype='char',
Copy link
Member Author

Choose a reason for hiding this comment

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

So, instead of catching the warning, I fix the call so it does not emit warning in the first place. Is this more desirable, @astrofrog ?


writeto(votable2, os.path.join(str(tmpdir), "through_table.xml"))
# W39: Bit values can not be masked
with pytest.warns(W39):
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing much I can do here without scope creep. I think it is correct behavior for writer to be more verbose than reader. But I have no desire to fix the data file so that it stops complaining about bit values and masking.

Copy link
Contributor

@tomdonaldson tomdonaldson left a comment

Choose a reason for hiding this comment

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

I have no concerns with the content or scope of this PR as it stands now.

@pllim
Copy link
Member Author

pllim commented Oct 3, 2019

Thanks!

@pllim pllim merged commit f725687 into astropy:master Oct 3, 2019
@pllim pllim deleted the tests-vo-warn-2 branch October 3, 2019 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants