g.extension: add tests for download from various sources

[ninsbl]

This PR adds test for downloading addons from:

• github
• gitlab
• bitbucket and
• the official addon repository as well as
• for MS Windows

Cause I was not sure if tests that download content from the internet are acceptable I deactivated tests by default. So devs would have to activate them by setting skip to False.

[ninsbl]

See also #625

[ninsbl]

Cause I was not sure if tests that download content from the internet are acceptable I deactivated tests by default. So devs would have to activate them by setting skip to False.

Any opinion on this?

[neteler]

Testing it with Python 3.9.4 shows:

testsuite/test_addons_download.py:19: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
  import imp

Could that be updated?

Then some tests fail since branch has a default setting but url=None, e.g.

https://github.com/OSGeo/grass/pull/1158/files#diff-826354815781b87e6d667944c79e46e0ae51bc1b9debf80295293494201951ddR137

which leads to

Traceback (most recent call last):
  File "/home/mneteler/software/grass_master/scripts/g.extension/testsuite/test_addons_download.py", line 136, in test_github_install_official_multimodule
    self.assertModule(
  File "/home/mneteler/software/grass_master/dist.x86_64-pc-linux-gnu/etc/python/grass/gunittest/case.py", line 1419, in assertModule
    self.fail(self._formatMessage(msg, stdmsg))
AssertionError: Running <g.extension> module ended with non-zero return code (1)
Called: g.extension(extension='i.sentinel', operation='add', prefix='/home/mneteler/software/grass_master/scripts/g.extension/testsuite/gextension_test_install_path', branch='main')
See the following errors:
...

        url   URL or directory to get the extension from (supported only on Linux and Mac)
...
     branch   Specific branch to fetch addon from (only used when fetching from git)
              default: main

ERROR: Option <branch> requires <url>

[ninsbl]

Then some tests fail since branch has a default setting but url=None, e.g.

https://github.com/OSGeo/grass/pull/1158/files#diff-826354815781b87e6d667944c79e46e0ae51bc1b9debf80295293494201951ddR137

ERROR: Option requires

Using run_command from grass.script does not throw errors related to branch and url, but any pygrass based call to g.extension does.

We should probably remove the requirement to specify url in g.extension if branch option is used (which comes with main as the default which causes the issues in the tests).

[wenzeslaus]

Using run_command from grass.script does not throw errors related to branch and url, but any pygrass based call to g.extension does.

That sounds like pygrass is broken, not the g.extension interface.

[wenzeslaus]

I was not sure if tests that download content from the internet are acceptable I deactivated tests by default.

This is tricky either way. Not running the tests when we have them? Unfortunate. Not for datasets and normal tests, but in general, downloading is okay in the CI and we do it (dependencies, sample dataset). It may fail time to time due to 3rd party issues and that's fine.

I would say, let's have it enabled and fix it when it doesn't work. (An alternative is having a separate GitHub Action or job which will do this special test.)

The URLs it is reaching should be somewhat reliable and perhaps under our control. For example, we have the alternative GRASS org on GitHub.

[wenzeslaus]

There is still couple of r.plus.example.

I don't get the reasons for the MS Win test. Can you comment more in the code?

r.clip, perhaps a candidate to be moved to core like v.clip? Maybe a different module would be a better match for this test. Also one Python and one C module might be a good idea.

[ninsbl]

There is still couple of r.plus.example.

Yet all tests pass locally. I think the module name is not relevant if you install from single module repositories, as the URL is build just with the repo name... But I wil change that so it is more clean...

I don't get the reasons for the MS Win test. Can you comment more in the code?

When writing the tests, I thought there were no tests on MS Windows. On MS Windows, installation is mainly downloading and extracting a zip file, so the tests checks the windows specific functions for that on Linux.
An issue with running this test on Windows may still be that addons are pre-compiled and installation may fail because of the version mismatch between GRASS compiled for testing and GRASS version compiled to build addon binaries... Which I guess is why OSGeo uses this patch:
https://github.com/jef-n/OSGeo4W/blob/a372e6fd3a5c9962146d1967a067c326ee814ef3/src/grass/osgeo4w/patch#L57-L64
Yet, I am not sure if that kicks in during tests of g.extension... Maybe the best way is to comment it out for now, then see how the testcase behaves on MS Windows (will try to test locally) and remove it if we can run the testcase on MS Widows as well...

r.clip, perhaps a candidate to be moved to core like v.clip? Maybe a different module would be a better match for this test. Also one Python and one C module might be a good idea.

Yes, agreed. Will change that.

[wenzeslaus]

I think the module name is not relevant if you install from single module repositories

Agreed.

I don't get the reasons for the MS Win test. Can you comment more in the code?

When writing the tests, I thought there were no tests on MS Windows.

Thanks, that makes sense.

An issue with running this test on Windows may still be that addons are pre-compiled and installation may fail because of the version mismatch between GRASS compiled for testing and GRASS version compiled to build addon binaries...

I didn't think about that. The check is performed when a module runs, but I don't think modules run during Windows installation, so we may be okay as long as the download links are resolved properly.

Which I guess is why OSGeo uses this patch:
https://github.com/jef-n/OSGeo4W/blob/a372e6fd3a5c9962146d1967a067c326ee814ef3/src/grass/osgeo4w/patch#L57-L64

Unfortunate situation. Well, good to know about it.

Maybe the best way is to comment it out for now, then see how the testcase behaves on MS Windows (will try to test locally) and remove it if we can run the testcase on MS Widows as well...

It is definitively more advanced test than I would write. I guess I would just add some secret flag (not a g.parser one) to g.extension, but this seems too be more general and it does not mess up the interface and tests can be outside of the already long module code. There might be value in testing things this way, e.g., you can test locally on Linux. On the other hand, if it makes things more complicated and the same functionality is easily tested elsewhere, I would not keep it for simplicity.

[wenzeslaus]

The commit message says add C module, but it changes the module tested in the official module test instead of having two tests. Intentional?

[ninsbl]

The failing test does not seem to be related to this PR.
Suggest to remove the relatively complex windows test function later if it should causes trouble.
The rest is hopefully addressed!?

[wenzeslaus]

This seems to have a lot of files which are not related to the PR and likely not really part of the source code at all. Accidental git -A?

[ninsbl]

This seems to have a lot of files which are not related to the PR and likely not really part of the source code at all. Accidental git -A?

Hm, it may have been due to a merge commit, but I am not sure. Tried to solve it with a hard reset. Hope that is an acceptable fix for the issue...

[ninsbl]

With OSGeo4W tests in CI, I am all fine with removing the MS Windows test function, if that is preferable.

[wenzeslaus]

Thanks. I have nothing too serious except perhaps one item. It could go in as is, but I think some things could be modernized further.

[ninsbl]

There are still some issues with the tests on MS Windows, but I will address them with a more concentrated crack down on unittest issues on that OS...