Add more tests of package traversal #906
Conversation
|
@LecrisUT this PR should capture some of the cases you mentioned in #808 (comment). Are there others that you would like to see added? |
|
Looks promising :). I'll need to draw it out on a paper to follow it 😅 , I'll come back to you after that. At first glance, I think the only part not covered is having both relative and absolute paths in the |
tests/test_editable.py
Outdated
| from importlib.readers import MultiplexedPath | ||
| import pkg | ||
| import pathlib | ||
| print({check}) |
There was a problem hiding this comment.
This should be assert instead of print. Could you also move check to be in-lined here instead of a parameter?
There was a problem hiding this comment.
Rather than asserting here, I print the boolean result and assert the string value outside because asserting inside a subprocess requires significantly more post-processing to handle cleanly.
There was a problem hiding this comment.
Could you elaborate on why it requires more post-processing, or what the output you get when doing that?
There was a problem hiding this comment.
I take it back. I originally said this because while debugging specific cases it was more challenging to extract information from subprocess exceptions than to simply parse inputs and outputs to run inside the environment, but in the final state the output is sufficiently readable as is. I've moved the assert.
Could you also move check to be in-lined here instead of a parameter?
I don't understand this suggestion. check is parametrized, so how would I inline it without duplicating the test?
There was a problem hiding this comment.
What I was suggesting is to not parametrize it, and instead write all of the checks explicitly in the isolated.execute part
There was a problem hiding this comment.
I guess we could do that. Would you just want each of the parametrizations to be a separate test, then, with all the parameters hardcoded within each test (and presumably in the name)?
There was a problem hiding this comment.
Not like that. Just put the asserts one after another and let it run until it fails the first time.
There is pytest_check if you want to get a report for multiple commands, but I don't think we will gain much by using that
There was a problem hiding this comment.
Oh OK yes I can do that, sure.
When you get a chance, let me know what you think of the latest changes and #906 (comment), then I can push the next update to this PR and address this request as well.
There was a problem hiding this comment.
Yes, busy week, but from Monday I will have time
There was a problem hiding this comment.
Regarding the coverage, right now it seems the import only cover python packages, the c methods must also be imported there.
Most of the imports only cover 1 level deep imports, and are a bit redundant. I would say to minimize it and comment what is being covered:
# pkg/__init__.py
# Covering importing one-level deep from 1st level pakage
from .py_mod import py_square1
from .c_mod import c_square1
# Coverging importing 2-levels deep from 1st level package
from .subA.py_mod import py_square2
from .subA.c_mod import c_square2# pkg/subA/__init__.py
# Covering importing one-level deep from 2nd level pakage
from .py_mod import py_square3
from .c_mod import c_square3
# Coverging importing relative paths backwards
from ..subB.py_mod import py_square4
from ..subB.c_mod import c_square4I suck at naming conventions, hope you can figure a good one.
|
On the overall structure of the files, I think it's a good skeleton to cover all the cases, we just need to cover the navigations within there. Just a few notes:
|
|
OK I think I've addressed most of the requests. Some notes:
|
There was a problem hiding this comment.
* I have not yet included any imports traversing up the hierarchy because those would cause circular imports. I'm not sure how you'd like that to be handled here.
You should be able to break the circularity, just limit the import in the __init__.py files. Specifically, I think you can eliminate the imports in sub_a and sub_d and use sub_b and sub_c to test relative imports upwards. Maybe some other names would be helpful to visualize which is the main tree that is tested (i.e. pkg, pkg.sub_b, pkg.sub_b.sub_c) and the auxiliary branches that are used to cover the navigation cases (e.g. ..sub_a, ...sub_a being called from the main tree)
* I added the top-level pure and extension modules but I don't know what you'd like me to do with them.
Just have them in a import statement at pkg/__init__.py should be fine
* I haven't added namespace packages yet. I don't understand the importance of the src vs installed cases that you want to address.
The difference is how editable installs handle it. When they are installed in the site_package, we can mostly ignore all package traversal since it would be handled by python std itself. If there is an error it's an error of the installation. When they are in src and they are handled by editable install, then we need to make sure that our script is configured appropriately and all navigation cases are covered. The testing of both installed and editable would just make it easier to figure out if it is indeed a editable install issue or not, and it's good to have parity confirmation in general.
Note that we should also cover the importlib.resources navigation with testfile as the target (one located in the CMake tree that you have and one located in the src tree)
You can write it as asserts in the __init__.py, e.g.
from importlib.resources import files
cmake_file = files("..") / "testfile"
assert cmake_file.read_text() == "This is the file"
Feel free to one-line it if needed.
This is a tall order, but do you know how to use draw.io? We could make a simple drawing of the tree and put some notes there, and have it under vcs here.
|
|
||
| python_add_library(emod MODULE emod.c WITH_SOABI) | ||
| install(TARGETS emod DESTINATION .) | ||
| install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/pmod.py" DESTINATION .) |
There was a problem hiding this comment.
| install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/pmod.py" DESTINATION .) |
This should not be needed, specify the wheel.packages in the pyproject.toml instead
There was a problem hiding this comment.
Could these be populated with an actual implementation. Probably:
def square(x: float): -> float
...| from .sub_b import emod_c | ||
|
|
||
| # Level two subpackages | ||
| from .sub_b import sub_c, sub_d |
There was a problem hiding this comment.
| from .sub_b import sub_c, sub_d | |
| from .sub_b import sub_c |
Let's focus the import on a single branch, and use the rest for navigation only.
This PR adds tests (including some xfailed ones) demonstrated patterns of package/subpackage access via import and importlib that are expected to work correctly for both normal and editable installs. Related to #807.