Fixturize Test Excel #26543
Fixturize Test Excel #26543
Conversation
WillAyd
added
Testing
Codecov Report
@@ Coverage Diff @@
## master #26543 +/- ##
==========================================
- Coverage 91.77% 91.76% -0.01%
==========================================
Files 174 174
Lines 50638 50638
==========================================
- Hits 46471 46467 -4
- Misses 4167 4171 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26543 +/- ##
==========================================
- Coverage 91.77% 91.76% -0.01%
==========================================
Files 174 174
Lines 50649 50649
==========================================
- Hits 46483 46479 -4
- Misses 4166 4170 +4
Continue to review full report at Codecov.
|
|
@simonjayhawkins I think looking a lot better after the monkeypatch. Let me know what you think |
|
|
||
| # FILE | ||
| localtable = os.path.join(self.dirpath, 'test1' + ext) | ||
| localtable = os.path.join(datapath("io", "data"), 'test1' + ext) |
There was a problem hiding this comment.
This was actually failing with the fixturized approach because the URL requires an absolute path, or else it raises a URLError and gets skipped.
Quite a few ways to do this but I figured just reusing the datapath fixture was easiest, especially since this is pending deprecation
| expected = read_excel(str_path, 'Sheet1', index_col=0) | ||
|
|
||
| abs_dir = os.path.abspath(self.dirpath) | ||
| path_obj = LocalPath(abs_dir).join('test1' + ext) | ||
| path_obj = LocalPath().join('test1' + ext) |
There was a problem hiding this comment.
Given the fixture changes to the data directory, this invocation without an argument resolves to there automatically without need to resolve to an absolute path
There was a problem hiding this comment.
much cleaner without the visual noise of the filepaths.
is the engine parameter of read_excel ignored when using ExcelFile? There seems to be a few tests passing if i use a bad engine in the fixture. I would have expected all the TestXlrdReader tests to fail. (except perhaps test_bad_engine_raises and similar)
it also appears to be an issue on master, so not introduced here.
can you check this out?
Yes ExcelFile has it's own engine parameter. If that's passed to read_excel along with another engine argument it should probably raise or warn. I'll open that as a follow up |
|
See #26566 |
so perhaps ExcelFile should be monkeypatched as well, so that the engine parameterisation applies to all tests? |
|
there is basically two changes here, the working directory and the engine parameterisation, maybe they should be kept seperate. the autouse fixture for the working directory is fine. maybe have the engine parameterisation as a seperate fixture, not make it autouse. then param_engine = pytest.mark.usefixtures('<fixturename>') near the top of the module and then decorate using @param_engine on just the tests to be parametrised ( excluding test_bad_engine_raises etc) |
|
A module level fixture would be great, though it's just going to take quite a few more PRs to get there, especially since a lot of the Writer test cases are strangely intertwined with reading. Left yet to go is:
Amongst potentially other things. What you described makes sense but it's going to be rather difficult to move that to the module level while the WriterBase is still subclassing SharedItems as things are convoluted and heavily intertwined where they don't need to be right now :-( The scope it's at now reflects current state so trying to minimize movement |
i wasn't suggesting moving the fixture. param_engine decorator defined at module level for clarity. this can be defined before the fixture since it's a pytest mark only. the fixture name is a string. but yes it could be defined at class level. in the past where i have only a few exceptions to a autouse fixture, i created another fixture to undo the monkeypatch and apply to the exceptions. that's a bit more complicated though. don't we now, as the PR stands, have parameterisation applied to tests that is ignored? and wasn't before and test output descriptions implies parametrisation. |
i think the tests that pass that shouldn't are different from master. so there is a change in test behavior here. i'll go back and thoroughly double check this. that's my concern. otherwise the changes are great. |
OK I think I follow. In the end we probably want separate objects for
I might not completely follow but generally I'd say no. The majority of parametrization here is for the engine, which most of the tests are using. The exceptions are tests in the WriterBase class that don't need to read in a file, but it's a mixed bag there; they don't belong in that class in any case so if we can decouple those from pure writing tests we can clean this up further, so I think that's one of the follow ups. Let me know if I misunderstood anything though; this module definitely warrants close review! |
Let me know what you see. I was getting this on master and this branch: ======== 966 passed, 12 skipped, 3 xfailed, 2 warnings in 30.45 seconds ======== |
|
if i change the fixture
to
i get.. 48 tests (only 8 actual test) passing. shouldn't they fail if the parametrisation is being applied? |
i should have said the tests that pass, after intentionally breaking the fixture. |
Ha I might have this backwards but note that that particular parametrization only applies to the reading tests. There are other classes like _WriterBase and even top level tests that won't be affected by changing the partial |
|
on master.. changing
to
gives so the test behavior is different. |
|
Great catch! But if anything I think it's indicative of an issue on master, where Can you run verbose output and see what the difference in passing in? My guess is that it's function in the ReadingTestsBase which previously used |
|
the tests that are not observing the parametrisation in this PR are these tests are also not failing (after intentianally breaking parameterisation) on master, so i've satisfied myself that the problem is not a new one. |
|
test_bad_engine_raises shouldn't parametrised. the other tests all use ExcelFile. there are some other tests that use ExcelFile that aren't appearing in the list because they are tests that will need to be split (future PR). they have a read_excel preceeding the ExcelFile and not showing with this testing of the tests. I'm happy to approve this PR on two grounds: firstly the problems are not introduced by this PR and secondly we are only testing one engine. before i do, do you want to see if you can easily monkeypatch ExcelFile? |
|
I don’t quite understand what you are asking for with the monkeypatch of ExcelFile - is that trying to solve a problem or expand coverage?
…Sent from my iPhone
On May 29, 2019, at 3:07 PM, Simon Hawkins ***@***.***> wrote:
test_bad_engine_raises shouldn't parametrised.
the other tests all use ExcelFile.
there are some other tests that use ExcelFile that aren't appearing in the list because they are tests that will need to be split (future PR). they have a read_excel preceeding the ExcelFile and not showing with this testing of the tests.
I'm happy to approve this PR on two grounds:
firstly the problems are not introduced by this PR and secondly we are only testing one engine.
before i do, do you want to see if you can easily monkeypatch ExcelFile?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
i give it a go in a short while, can partial be used on the Class constructor? |
|
I believe partial is only for functions not classes. Is your question to convert |
i was thinking we could add another monkeypatch.setattr to the current cd_and_set_engine fixture
not on the init method then? |
on master the following tests are all using read_excel directly and not self.get_exceldf where the parameterisation was being applied. hence not being parametrised as intended. So this PR has fixed those issues. |
is working on all the tests apart from test_read_xlrd_book (and test_bad_engine_raises), i'll keep digging |
|
the non-foo version! |
so this test is included in the parametrisation of the class, but specifies the engine explicitly as engine = "xlrd" removing that works. |
|
The only reader right now is xlrd. If the monkey patching you are looking at is only for the readers I think it should wait until another PR or until we actually get another reading engine implemented. |
There was a problem hiding this comment.
ok. nothing's broken that wasn't already broken. in fact this has fixed as few issues.
I do like the way partial has been used for the parametrisation. but maybe it introduces another layer of magic. I think that the current status shows that this is error prone.
so maybe at some point (not in this PR), this module should be changed to the tried and tested parameterisation method, include engine in the test function signature and just use engine=engine where required.
|
lgtm. merge when ready @simonjayhawkins and @WillAyd |
|
thanks @WillAyd |
|
Thanks for the review @simonjayhawkins ! |
Continued simplification of this module by moving towards pytest idiom. Here I have eliminated any test instance methods and replaced with fixtures