Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add comprehensive tests for pygmt.clib.loading.clib_full_names #872

Merged
merged 24 commits into from
Feb 26, 2021

Conversation

seisman
Copy link
Member

@seisman seisman commented Feb 12, 2021

Description of proposed changes

This PR adds comprehensive tests for the function pygmt.clib.loading.clib_full_names. Following cases are tested using monkeypatch.

GMT_LIBRARY_PATH PATH
case 1 undefined empty
case 2 defined empty
case 3 undefined contains GMT's bin
case 4 defined contains GMT's bin
case 5 defined but incorrect contains GMT's bin

Some notes:

  1. The tests assume that the CI environment (or our local testing environment) can find the gmt command. Otherwise these tests fail. The assumption is valid for our current CI environments, so I prefer not to make the tests too complicated.
  2. The tests only check if clib_full_names() returns a generator and the expected list of names. In these tests, the GMT library is not loaded.
  3. The tests increase the coverage of pygmt/clib/loading.py by 2 more lines, thus now it's 💯
  4. There is still one rare case that is not tested. On Windows, if GMT's bin path is included in PATH, but GMT's library gmt.dll isn't, then the find_library function (which searches DLLs in PATH) won't find gmt.dll. Thus the length of the returned names list will be -1.

Fixes #

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@seisman seisman marked this pull request as ready for review February 16, 2021 21:56
@seisman
Copy link
Member Author

seisman commented Feb 17, 2021

@weiji14 I'd appreciate it if you can give it a review. The latest CI runs (some are skipped by [skip ci]) are available at https://github.com/GenericMappingTools/pygmt/runs/1915712307.

Please read the PR description first. For the point 4, I think it's possible to simulate this case by monkeypatching the find_library function, but I still don't know how to do it.

There is still one rare case that is not tested. On Windows, if GMT's bin path is included in PATH, but GMT's library gmt.dll isn't, then the find_library function (which searches DLLs in PATH) won't find gmt.dll. Thus the length of the returned names list will be -1.

@seisman seisman requested a review from weiji14 February 17, 2021 04:25
@seisman seisman added the maintenance Boring but important stuff for the core devs label Feb 17, 2021
@seisman seisman added this to the 0.3.1 milestone Feb 17, 2021
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read the PR description first. For the point 4, I think it's possible to simulate this case by monkeypatching the find_library function, but I still don't know how to do it.

Just one quick comment for now on removal of a fixture. I'll need a bit of a think on how to handle the point 4 case.

There is still one rare case that is not tested. On Windows, if GMT's bin path is included in PATH, but GMT's library gmt.dll isn't, then the find_library function (which searches DLLs in PATH) won't find gmt.dll. Thus the length of the returned names list will be -1.

Why is the length of the returned list from clib_full_names -1? Shouldn't it be 0?

pygmt/tests/test_clib_loading.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member Author

seisman commented Feb 17, 2021

There is still one rare case that is not tested. On Windows, if GMT's bin path is included in PATH, but GMT's library gmt.dll isn't, then the find_library function (which searches DLLs in PATH) won't find gmt.dll. Thus the length of the returned names list will be -1.

Why is the length of the returned list from clib_full_names -1? Shouldn't it be 0?

Sorry, the description is confusing. For this case (i.e., gmt.dll and gmt.exe are not in the same directory, and PATH includes GMT's bin path), the number of names will be one less compared to normal cases.

In normal cases, the list of names would be:

["/path/from/gmt_library_path/gmt.dll", 
 "/path/from/gmt-show-library/gmt.dll", 
 "/path/from/ctypes-find-library/gmt.dll",
 "gmt.dll",
 "gmt_w64.dll",
 "gmt_w32.dll",
]

In this rare case, "/path/from/ctypes-find-library/gmt.dll" is not returned, because find_library won't find gmt.dll in PATH.

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@seisman seisman changed the title WIP: Add comprehensive tests for pygmt.clib.loading.clib_full_names Add comprehensive tests for pygmt.clib.loading.clib_full_names Feb 18, 2021
@seisman
Copy link
Member Author

seisman commented Feb 24, 2021

Ping @weiji14 to give it a review when possible.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still one rare case that is not tested. On Windows, if GMT's bin path is included in PATH, but GMT's library gmt.dll isn't, then the find_library function (which searches DLLs in PATH) won't find gmt.dll. Thus the length of the returned names list will be -1.

Why is the length of the returned list from clib_full_names -1? Shouldn't it be 0?

Sorry, the description is confusing. For this case (i.e., gmt.dll and gmt.exe are not in the same directory, and PATH includes GMT's bin path), the number of names will be one less compared to normal cases.

In normal cases, the list of names would be:

["/path/from/gmt_library_path/gmt.dll", 
 "/path/from/gmt-show-library/gmt.dll", 
 "/path/from/ctypes-find-library/gmt.dll",
 "gmt.dll",
 "gmt_w64.dll",
 "gmt_w32.dll",
]

In this rare case, "/path/from/ctypes-find-library/gmt.dll" is not returned, because find_library won't find gmt.dll in PATH.

Sorry, I've been meaning to log on to Windows to test the special case but too many things on my plate at the moment. Up to you, but I can either approve this PR now as the other cases look covered (and we handle the special case one another time), or keep this PR open for a little longer.

@seisman
Copy link
Member Author

seisman commented Feb 26, 2021

approve this PR now as the other cases look covered (and we handle the special case one another time)

The special case is very rare and I believe it would be difficult for you to reproduce the case. So I prefer to this option if the current tests look good.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agree that it is quite rare, and probably only happens with a very complicated setup (but will see if someone complains). Good to merge then.

@seisman seisman merged commit a98fb85 into master Feb 26, 2021
@seisman seisman deleted the gmt-library-tests branch February 26, 2021 02:47
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…icMappingTools#872)

This PR adds comprehensive tests for the function pygmt.clib.loading.clib_full_names. 

See GenericMappingTools#872 for details.

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants