Remove SharedItems from test_excel #26579
Conversation
WillAyd
added
Testing
| def test_mixed(self, merge_cells, engine, ext): | ||
| self.mixed_frame.to_excel(self.path, 'test1') | ||
| def test_mixed(self, merge_cells, engine, ext, frame): | ||
| mixed_frame = frame.copy() |
There was a problem hiding this comment.
This was previously a global variable. I didn't convert to a fixture like the rest simply because this is the only test that uses it
| frame.to_excel(self.path, 'test1', | ||
| index_label=['test'], | ||
| merge_cells=merge_cells) | ||
| df = (DataFrame(np.random.randn(10, 2)) >= 0) |
There was a problem hiding this comment.
So as not to confuse the fixture with a local variable I changed frame to df here (and in test_*_types tests)
|
@simonjayhawkins thanks for the thorough review - definitely making this PR a lot better. Latest push I think should address your callouts |
| def test_ts_frame(self, *_): | ||
| df = tm.makeTimeDataFrame()[:5] | ||
| def test_ts_frame(self, tsframe, *_): | ||
| df = tsframe |
There was a problem hiding this comment.
Rather than duplicating logic leveraged the fixture here
Codecov Report
@@ Coverage Diff @@
## master #26579 +/- ##
===========================================
- Coverage 91.87% 41.8% -50.08%
===========================================
Files 174 174
Lines 50692 50692
===========================================
- Hits 46575 21190 -25385
- Misses 4117 29502 +25385
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26579 +/- ##
==========================================
- Coverage 91.87% 91.86% -0.02%
==========================================
Files 174 174
Lines 50692 50692
==========================================
- Hits 46575 46569 -6
- Misses 4117 4123 +6
Continue to review full report at Codecov.
|
| _mixed_frame['foo'] = 'bar' | ||
|
|
||
| @pytest.fixture | ||
| def frame(float_frame): |
There was a problem hiding this comment.
great, next time around can you add some doc-strings, and/or make more meaningful names
|
thanks @WillAyd |
Pretty big step towards logically partitioning tests in this module - replaced SharedItems and instance attributes with fixtures. Should make subsequent test reorg easier
@simonjayhawkins