REF: test_to_latex #36528
REF: test_to_latex #36528
Conversation
Split tests into multiple test functions in test_to_latex.
| assert result == expected | ||
|
|
||
| df = DataFrame.from_dict( | ||
| @pytest.fixture | ||
| def multiindex_frame(self): |
There was a problem hiding this comment.
Just as a matter of convention can you move the fixture to the top of the module rather than in between the tests? (though this was nice to keep here for review)
There was a problem hiding this comment.
I moved the fixtures to the top of the class. If I take one more step in a separate PR, I would split the large test class into multiple classes and distribute relevant fixtures between them.
jreback
added
IO LaTeX
|
yeah agree with @WillAyd comment as well. Note that would be also fine to organize these into different classes (if that's useful). same / followon ok with this ping on green. |
Yes, I though of re-organizing the tests into separate classes. I would prefer to do that in a separate PR, after this one. What is the policy on the test functions naming? I mean if we organize tests in the appropriate classes, then would that be necessary for us to keep Similarly, if we have a separate class on, let's say multiindex, then I would rather remove |
| with open(path) as f: | ||
| assert float_frame.to_latex() == f.read() | ||
|
|
||
| def test_to_latex_to_file_utf8_with_encoding(self, float_frame): |
There was a problem hiding this comment.
float_frame not needed in this test.
| # test with utf-8 and encoding option (GH 7061) | ||
| df = DataFrame([["au\xdfgangen"]]) | ||
| with tm.ensure_clean("test.tex") as path: | ||
| df.to_latex(path, encoding="utf-8") | ||
| with codecs.open(path, "r", encoding="utf-8") as f: | ||
| assert df.to_latex() == f.read() | ||
|
|
||
| def test_to_latex_to_file_utf8_without_encoding(self, float_frame): |
| class TestToLatex: | ||
| def test_to_latex_filename(self, float_frame): | ||
| @pytest.fixture | ||
| def df_caption_label(self): |
There was a problem hiding this comment.
fixtures should have docstrings xref #19159
| ) | ||
|
|
||
| @pytest.fixture | ||
| def df_caption_label_longtable(self): |
There was a problem hiding this comment.
might make the tests more readable (no tuple unpacking) if frame, caption, and label were composable fixtures from a longtable fixture yielding True/False
because fixtures returning tuples can be undesirable for several reasons, pytest-cases has a helper to overcome this https://smarie.github.io/python-pytest-cases/pytest_goodies/#unpack_fixture-unpack_into.
so should maybe consider avoiding creating fixtures returning tuples.
There was a problem hiding this comment.
I tried to make it less painful by using namedtuple.
Anyway, I checked on the fixture unpacking (thank you for mentioning it, I was not aware of that).
So far, for the cases concerned, I do not see why it is better than separate fixtures. I mean, that is certainly beneficial if the fixture contains parametrization, but the ones involved do not have parametrization.
So, I decided to supply separate fixtures for dataframe, captions and labels.
What do you think about it?
There was a problem hiding this comment.
changes look good.
So far, for the cases concerned, I do not see why it is better than separate fixtures. I mean, that is certainly beneficial if the fixture contains parametrization, but the ones involved do not have parametrization.
In a follow-on could maybe parameterise the caption and label the fixtures on longtable True/False and combine tests.
it is sometimes better to parameterise the tests firsts and then extract common paramterisation into fixtures.
|
thanks @ivanovmg very nice. so a couple of things for followons.
|
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffRefactor/clean-up
test_to_latex.py.