TST: xmatch test improvements#2468
Conversation
Using a module level `pytest` fixture avoids quering the remote server multiple times during a single test session.
The `pathlib` module was introduced to the Python standard library in version 3.4 and often provides a less verbose way of specifying paths than `os.path`. Furthermore, the `pytest` `tmp_path_factory` fixture allows creating temporary directories as `Path` instances in a simple but robust way.
Codecov Report
@@ Coverage Diff @@
## main #2468 +/- ##
=======================================
Coverage 62.92% 62.92%
=======================================
Files 133 133
Lines 17302 17302
=======================================
Hits 10888 10888
Misses 6414 6414 📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
|
||
|
|
||
| DATA_DIR = os.path.join(os.path.dirname(__file__), 'data') | ||
| DATA_DIR = Path(__file__).parent / "data" |
There was a problem hiding this comment.
would this work on windows? Could we maybe keep the os.path.join approach here and everywhere below to handle separators?
There was a problem hiding this comment.
I have added a third commit that adopts pathlib.Path in local tests. You can see that the CI tests indeed succeed on Windows too.
There was a problem hiding this comment.
yes, in VM-ed windows. Would it also be true in real user cases? I would really rather avoid getting rid of the proper way of handling separators just because it works for us in the one case we set up in CI (historically it indeed was a problem).
I'm not against using path-like objects, but would rather see they are stuck into os.path.join (it can receive Path objects) rather than having a hard-wired / as a separator.
pinging @pllim as the windows expert to comment whether my concern is outdated.
There was a problem hiding this comment.
/ operation for Path should be OS agnostic. So 👍 from me. 😸
There was a problem hiding this comment.
OK, let's have this in then. fyi I still don't like this approach, and would prefer to stick with os.path (most of all as that's what we use elsewhere (along with the odd manual string handling, all of which should be cleaned up eventually), but the PR fixes part of the problems we had, so will live with it.
The `pathlib` module was introduced to the Python standard library in version 3.4 and often provides a less verbose way of specifying paths than `os.path`.
The first commit makes the remote tests run faster by using a
pytestmodule level fixture to cache remote data.The second commit uses the
pytesttmp_path_factoryfixture to ensure that a file created during remote tests is always created in a temporary directory. This contributes towards #2107.The third commit adopts
pathlib.Pathin the local tests.