Refactor xfail tests to avoid storing baseline images#603
Conversation
pygmt/tests/test_basemap.py
Outdated
| fig_ref = Figure() | ||
| with Session() as lib: | ||
| lib.call_module("basemap", "-R0/360/0/1000 -JP6i -Bafg") |
There was a problem hiding this comment.
The reference image is created by directly calling the low-level API function call_module.
There was a problem hiding this comment.
I like the concept in general (testing against the low level GMT API), but this can get complicated real fast for some modules that output to a file (see e.g. @liamtoney's example at #364 (comment) for grdgradient). Though to be honest, that's probably not what this PR is about.
Should we have a convenience function to wrap around this low-level API better? E.g. something like pygmt.gmt(module="basemap", arg_str="-R0/360/0/1000 -JP6i -Bafg") that removes the with` statement.
There was a problem hiding this comment.
but this can get complicated real fast for some modules that output to a file (see e.g. @liamtoney's example at #364 (comment) for
grdgradient).
Most tests are simple, testing only one aspect of functions. #364 (comment) example may be the most complicated one, but we won't use virtual files for the references images.
Should we have a convenience function to wrap around this low-level API better? E.g. something like
pygmt.gmt(module="basemap", arg_str="-R0/360/0/1000 -JP6i -Bafg") that removes thewith` statement.
I believe @leouieda is against it (#122 (comment)).
There was a problem hiding this comment.
Most tests are simple, testing only one aspect of functions. #364 (comment) example may be the most complicated one, but we won't use virtual files for the references images.
True, but as you mentioned, this way of testing isn't very Pythonic or new contributor friendly. We can definitely think of a better solution that doesn't involve clib here, just need a bit more time, and maybe have a look around some other projects like GMT.jl and matplotlib to see how they do it.
I believe @leouieda is against it (#122 (comment)).
Ah yes, I thought I saw this somewhere before, thanks for linking. Definitely hold on the pygmt.gmt/fig.rawcommand feature then.
pygmt/tests/test_basemap.py
Outdated
| fig_test = Figure() | ||
| fig_test.basemap(R="0/360/0/1000", J="P6i", B="afg") | ||
|
|
||
| fig_ref = Figure() | ||
| with Session() as lib: | ||
| lib.call_module("basemap", "-R0/360/0/1000 -JP6i -Bafg") |
There was a problem hiding this comment.
Just as a throwaway idea, what about testing that the long aliases and list-to-str conversion works? I.e. have fig_test use long aliases like region, and have fig_ref use the short alias like R.
| fig_test = Figure() | |
| fig_test.basemap(R="0/360/0/1000", J="P6i", B="afg") | |
| fig_ref = Figure() | |
| with Session() as lib: | |
| lib.call_module("basemap", "-R0/360/0/1000 -JP6i -Bafg") | |
| fig_test = Figure() | |
| fig_test.basemap(region=[0, 360, 0, 1000], projection="P6i", frame="afg") | |
| fig_ref = Figure() | |
| fig_ref.basemap(R="0/360/0/1000", J="P6i", B="afg") |
There was a problem hiding this comment.
Sounds a clever idea. I believe we are confident that arguments like R="0/360/0/1000", J="P6i", B="afg" will be converted to "-R0/360/0/1000 -JP6i -Bafg", thus, fig_ref.basemap(R="0/360/0/1000", J="P6i", B="afg") is the same as lib.call_module("basemap", "-R0/360/0/1000 -JP6i -Bafg").
There was a problem hiding this comment.
Cool, if you want to update this PR to fix the failing 'polar', 'logo' (as mentioned at #451 (comment)) tests, and any other ones that pop up (I saw a few more upstream GMT changes already...), I'll review it later.
There was a problem hiding this comment.
OK. I'll work on this PR.
20b0730 to
23494f0
Compare
23494f0 to
cc7c6b3
Compare
cc7c6b3 to
db18f4b
Compare
weiji14
left a comment
There was a problem hiding this comment.
A lot to review, but it's looking good! Just a couple of comments.
The two xfailing tests in test_config are not refactored. There is no easy way to generate references images for them.
Images of those xfailing/failing tests are no longer needed, but this PR doesn't delete them from the repository.
Yes, will need to remember to do these in the future as separate PRs.
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Description of proposed changes
This PR refactors the xfail/fail tests to let the tests pass and avoid storing baseline images.
The baseline images are generated by calling the same modules using plain text or grid files are input, and using single-character arguments.
Address #451.
Remaining issues:
test_configare not refactored. There is no easy way to generate references images for them.TODOs for future PRs:
pygmt/test/baselinesdirectory as baselines.Reminders
make formatandmake checkto make sure the code follows the style guide.doc/api/index.rst.