Skip to content

Conversation

@wmwv
Copy link
Contributor

@wmwv wmwv commented Aug 11, 2016

Adds a new test test_write_read.py that tests that all test_defs samples in test_write.py can be successfully written (to a StringIO object) and read back in to recover the correct data.

Note that adding this test creates 3 new failures. This is a "success" of this feature branch. But will thus break master passing its tests.

The failures are

The first two are the motivating reasons why I wrote this round-tripping test in the first place, so it's great that only other one thing came up.

  • Should I xfail these for now?

[*] The log shows 12 failures, but these are each of the key 3 failures failing in each of fast_writer=True, False and in each of the io.ascii and table.Table interfaces.

wmwv added 13 commits August 11, 2016 11:39
Use a hard-coded 'debug' variable to control level of output
of tests in test_read_write.py
The writers and readers need slightly different kwargs.
Use 'Writer' to seed 'Reader' or to define 'format'.
Strip out kwargs that don't make sense to Reader
  or would confuse Reader (e.g., 'delimiter=None').
'create_reader_kwargs_from_writer_kwargs'
creates a test_def with ['kwargs'] appropriate for Reading.
by copying from a test_def, removing keys from hardcoded list of names,
and handling the special case of 'delimiter=None'.
Each tested member of test_write.test_defs is now tracked as a separate test
Each of 'io.ascii' and 'table.Table' is now a separate test.
This routine tests if a table can be written, and then read back.
Thus the name test_write_read.py better reflects the order.
We can write HTML without BeautifulSoup
but we can't read it back in.
@wmwv
Copy link
Contributor Author

wmwv commented Aug 11, 2016

@hamogu Here's the test round-trip write-read function. I would very much welcome your comments and feedback.

@wmwv
Copy link
Contributor Author

wmwv commented Aug 11, 2016

@taldcroft I wrote this round-tripping test function for io.ascii to test that astropy.io.ascii (and the astropy.table.Table interface to io.ascii) can read what it writes. @hamogu kindly offered to take a look at it.

I appreciate your support of this concept and would welcome your feedback as well.

]

test_def_masked_fill_value = [
test_defs_masked_fill_value = [
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? It sees to me that "test_def" is jsut as good as "test_defs" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency with test_defs and test_defs_no_data. The prefix test_defs denotes a collection, while the individual test definition is the singular test_def.

"Pluralize test_defs_masked_fill_value for consistency"

@hamogu
Copy link
Member

hamogu commented Aug 12, 2016

I think this is a good think to test, specifically as we add features (e.g. the LaTeX writer used to round-trip in the past before we added support for units).

Can we add fixes for the test failures to this PR though? It's not going to get merged with the tests passing. You can say "But it's already broken, the tests only confirm that." The problem is that then all tests for all other PRs would also show a "failed" status which makes it a lot harder to spot the really new failures.
Alternatively, you could mark these tests as "expected to fail", and activate them one by one in other PRs that fix round-tripping, but it would be a lot better to fix the problem when it's found. And, at least for the LaTeX tables you have the fix already in another PR.

@hamogu
Copy link
Member

hamogu commented Aug 12, 2016

Lastly, can you squash the commits into one or two? (Let me know if you don't know how. I always for get, but I know where to look it up.)

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