BF: Resolve path to tools correctly. - #147
Conversation
Follow up from tee-ar-ex#144 This is causing failures only when testing sdist, because it can only be tested in the context of an installation. Here the following code: ``` TOOLS_DIR = Path(__file__).resolve().parents[2] / "tools" ``` resolved incorrectly, because the test file is located at `trx/tests/test_update_switcher.py`, not `tests/test_update_switcher.py` So, going up 2 parents from that location would be wrong and raises that error in the CI.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
==========================================
- Coverage 86.86% 86.40% -0.47%
==========================================
Files 14 15 +1
Lines 2970 3031 +61
==========================================
+ Hits 2580 2619 +39
- Misses 390 412 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@skoudoro : if you get a chance to take a look and give this a 👍, I'd appreciate it |
| import pytest | ||
|
|
||
| TOOLS_DIR = Path(__file__).resolve().parents[2] / "tools" | ||
| TOOLS_DIR = Path(__file__).resolve().parent.parent.parent / "tools" |
There was a problem hiding this comment.
Did he work for you ? Despite the change, it is still failing for me
There was a problem hiding this comment.
Works now with the recent commit
|
I think I see what is happening, but to be honest, I am not even sure why we are testing this functionality that is not directly related to TRX in the context of this library. I am inclined to remove the test altogether. |
|
Barring that, I think that we'd need to move the "tools" folder into the trx module, so that it gets properly installed. |
This is so that it can be installed and tested properly. Also updates docbuild workflow appropriately.
|
Last commit 40d28ae implements the latter approach by moving tools into trx/tools and updating workflow accordingly |
| utils = [ | ||
| "dipy", | ||
| "dipy >= 1.12", | ||
| "fury >= 0.10.0, < 2.0.0" |
There was a problem hiding this comment.
can you add
[tool.setuptools.package-data]
trx = ["tools/*.py"]
and also 'tools' to autoapi_ignore. to avoid to see this in the API.
Also, to remove from codecov ( trx/tools/*). because it decrease it for no reason
Not sure that I follow the logic here. This is being tested, so we want to at least make sure that coverage doesn't go down over time, no? |
Yes sorry, ignore, not sure why I miss those tests when looking at the files. so all good |
2 participants
Follow up from #144
This is causing failures only when testing sdist, because it can only be tested in the context of an installation.
Here the following code:
resolved incorrectly, because the test file is located at
trx/tests/test_update_switcher.py, nottests/test_update_switcher.pySo, going up 2 parents from that location would be wrong and raises that error in the CI.Also, pins DIPY dependency, which needs to be >=1.12, based on running the tests locally with 1.11, which did not work.