Move inline tests to a dedicated testing module #174
Conversation
pieleric
force-pushed
the
ctypes-move-test-cases-to-ci
branch
2 times, most recently
from
e877316
to
554fcd2
Compare
|
Flake8 is mad at the code: |
djhoese
changed the title
There was a problem hiding this comment.
This looks good to me. I tried changing the title to match what I would expect in a changelog (I don't remember if we auto-generate changelogs for pylibtiff but still). Not sure "inline" is the right word. Once flake8 issues are resolved we'll see how tests go.
Yes... I also saw that. I'm a bit confused why there are so many errors while it used to pass in the original file. I just copy-pasted that code! |
pieleric
force-pushed
the
ctypes-move-test-cases-to-ci
branch
from
554fcd2
to
c617dec
Compare
Try using |
|
About flake8 ignoring the old code, I wonder if it skipped over the functions because they started with |
pieleric
force-pushed
the
ctypes-move-test-cases-to-ci
branch
from
c617dec
to
0886300
Compare
I've investigated it a little bit. It turns out flake8 is entirely disabled on that file. That's "thanks" to this line: pylibtiff/libtiff/libtiff_ctypes.py Line 9 in d9c97de flake8 doesn't allow disabling a specific error for an entire file. So it just reads it as |
pieleric
force-pushed
the
ctypes-move-test-cases-to-ci
branch
from
0886300
to
8e5194a
Compare
Pull Request Test Coverage Report for Build 7576518304
💛 - Coveralls |
|
You can definitely do single rule ignores for a file: https://flake8.pycqa.org/en/latest/user/options.html#cmdoption-flake8-per-file-ignores Just not the way it was done there. |
Instead of keeping the test cases at the end of libtiff_ctypes.py, place them in test_libtiff_ctypes.py. The main advantage is that they should be called automatically during CI. It also reduces the actual library code, which avoids unnecessarily loading this code every time the library is used.
pieleric
force-pushed
the
ctypes-move-test-cases-to-ci
branch
from
8e5194a
to
d678786
Compare
There was a problem hiding this comment.
This looks good to me. It is definitely better than it was so thanks a ton for doing this work. I think in the long run it'd be nice to separate out those test file creation steps into a dedicated helper function that is used by the write test(s) and the read test(s). It could even return the actual filename/path of the file(s) it just wrote. Right now there is still some "I know to read this filename because of prior knowledge about the function I just called to write it" magic going on...but that's fine. Like I said, still way better than before. Thanks.
As discussed in the review of PR #173, it's better to have the test cases in the dedicated test file, because that allows to run them automatically in CI.
This PR does just that. I've dropped the possibility to run some of the test cases on different tiff files, as it's now focused to be run with pytest. The hard-coded filenames work fine on Linux, but I'm not sure they will do as well on Windows. The CI results should help to find out...