Repository navigation
Conversation
wenzeslaus
left a comment
wenzeslaus
left a comment
There was a problem hiding this comment.
This looks good, but the user's expectation would probably be also looking at GRASS_ADDON_PATH here.
|
Given this is based on a feature request, there is no test coverage. There are some g.extension tests where you can add this as an additional step conforming that GRASS_ADDON_BASE actually works. For GRASS_ADDON_PATH, it seems like the best solution is to create pytest-based test and create couple scripts in different dirs. |
222b167 to
0e44f4a
Compare
|
I have added a few lines to Do let me know if anything is incorrect or if any changes are needed. @wenzeslaus The Windows 2022 build is failing due to HTTP 404 errors, which appears unrelated to my changes; my local tests are passing successfully. |
wenzeslaus
left a comment
wenzeslaus
left a comment
There was a problem hiding this comment.
The pytest has a single test, no? Rework it to yield session instead of tools.
|
Currently there's only one test file right now (excluding conftest.py), But I have simplified the fixture to yield session directly which is good to have if we add some tests later. I moved the Tools logic into the test itself; shouldn't make much difference performance-wise since it's just used once. |
wenzeslaus
left a comment
wenzeslaus
left a comment
There was a problem hiding this comment.
This looks great now. Thank you.
We should merge only after the Windows CI is back since this actually needs the Windows check.
|
The windows extensions build server isn't updated to be able to build for 8.5 (released) and 8.6 (dev). So your test will still continue to fail for the time being. |
| from grass.tools import Tools | ||
|
|
||
|
|
||
| @pytest.mark.skipif(sys.platform.startswith("win"), reason="Temporarily disable tests.") |
There was a problem hiding this comment.
What about keeping the test disabled?
wenzeslaus
left a comment
wenzeslaus
left a comment
There was a problem hiding this comment.
I think the code change still holds and it is mergeable.
The behavior update is cross-platform, so we don't need to hold this because of the Windows CI failures, no?
This PR addresses issue mentioned in #3885
I have tested and verified that the output now includes installed extensions in the returned set.