CI: Address Windows specific issues in testsuite #2616

ninsbl · 2022-10-28T21:10:05Z

Replaces: #1683

ninsbl · 2022-10-31T21:51:32Z

With this PR success-rate of unit tests on MS Windows is up to 87% (from 80%), with 243 tests passing (almost as many as on Linux). There are a couple of tests left (e.g. i.maxlik or v.what tests), where I suspect that either tests (or assertions in the testsuite (e.g. with v.what) are not really cross platform. For the rest we may start looking at the module code to check if the test failure actually is a bug.
An example, where I here fixed the test, but test failure actually may be a bug is the v.in.lidar module. That module seems to behave differently on MS Windows and Linux with regards to projection handling and that may be actually a bug...

Anyway, I am happy about all sorts of feedback on this PR.

wenzeslaus · 2022-11-01T01:44:47Z

gui/wxpython/core/testsuite/toolboxes.sh

+python3 $GISBASE/gui/wxpython/core/toolboxes.py doctest
+python3 $GISBASE/gui/wxpython/core/toolboxes.py test


Do we have GRASS_PYTHON defined here or not? If yes, that might be the best choice, although I would prefer solution where plain python just works.

wenzeslaus · 2022-11-01T01:46:39Z

gui/wxpython/core/toolboxes.py

-    import __builtin__
+    if sys.version_info[0] == 2:
+        import __builtin__

-    __builtin__._ = new_translator
+        __builtin__._ = new_translator


The "underscore" issue should still apply, so I would be surprised if doing it just for 2 is enough. Besides that, if there is 2-only code somewhere, it should be deleted. (But I think that's not the case here.)

Right. We do not support Python 2 anymore, so it is probably even better to remove Python 2 specific code to cleanup (also other places where Python 2 code pops up)?

wenzeslaus · 2022-11-01T01:48:59Z

lib/init/testsuite/test_grass_tmp_mapset.py

@@ -30,13 +30,16 @@ class TestTmpMapset(unittest.TestCase):
    """Tests --tmp-mapset option of grass command"""

    # TODO: here we need a name of or path to the main GRASS GIS executable
-    executable = "grass"
+    executable = shutil.which("grass")


I would think only shutil.which or shell=True are needed for Windows, not both. I would prefer shutil.which or some equivalent of sys.executable in the grass package (e.g., grass.script.setup.executable).

I see. Unfortunately, both shutil.which and shell do not seem to help. Maybe better to switch to subprocess.run?

subprocess.run is just an interface to Popen like all the others. I don't expect it would help. shutil.which needs grass to be on path to work which is likely not fulfilled.

This test is now unique - we don't have other tests which test the executable - we we may have more in the future or even write such code, so perhaps ignoring it now and adding grass.script.setup.executable later is the way to go.

This is one the of things for a separate PR. Getting the grass.script.setup.executable piece working would be nice to have. get_install_path is perhaps a good starting point (Python package path is known, path to installation is not). Lazy-loading seems appropriate, but there are still no properties for modules yet (?), but there is getattr.

wenzeslaus · 2022-11-01T01:51:17Z

python/grass/gunittest/gmodules.py

+    # Make sure that universal newlines are returned
+    output = (
+        output.replace(os.linesep, "\n")
+        if os.linesep != "\n" and type(output) == str


type(output) is str?

wenzeslaus · 2022-11-01T01:52:26Z

raster/r.mfilter/testsuite/test_r_mfilter.py

@@ -1,4 +1,4 @@
-from tempfile import NamedTemporaryFile
+import grass.script as gscript


import grass.script as gs

wenzeslaus · 2022-11-01T01:53:37Z

raster/r.mfilter/testsuite/test_r_mfilter.py

@@ -184,7 +184,7 @@ class TestNeighbors(TestCase):

    def create_filter(self, options):
        """Create a temporary filter file with the given name and options."""
-        f = NamedTemporaryFile()
+        f = gscript.tempfile(create=False)


Returns string, the original is an IO object.

wenzeslaus · 2022-11-01T01:54:50Z

scripts/g.extension/testsuite/test_addons_download.py

+                # Prebuild C-modules for MS Windows are unlikely to run
+                # on fresh compiled GRASS GIS


I thought this should fail at g.extension step already.

Addons are pre-build for MS Windows also for the development version:
https://wingrass.fsv.cvut.cz/grass83/addons/grass-8.3.dev/logs/
However, getting the addon-help fails because build versions differ...

Oh, I missed what test_github_install_official is. That makes sense. Mention the different build versions in the comment.

wenzeslaus · 2022-11-01T01:56:08Z

vector/v.edit/testsuite/select_all_flag.sh

@@ -15,7 +16,7 @@ v.edit -r map=vedit_test tool=catadd layer=2 cats=10

 expected="10
 10"
-out=$(v.category option=print layer=2 input=vedit_test)
+out=$(v.category option=print layer=2 input=vedit_test| tr -d '\r')


BTW, skipping all .sh-based tests on Windows would be a fair solution I think.

I have no strong opinion here. MSYS has been dropped, so it is very unlikely that users will run shell scripts in real world cases. And I am not sure this is a real MS Windows test in that sense.
However, most of the shell tests work. So maybe it does not hurt to have them running for MS Windows as well...

Okay, please add comment that the \r part is for Windows, so we know when to remove it.

However, I think the role of Bash/sh/... tests is to provide option for "quick and dirty" tests and for very specific cases, so supporting only Linux or unix is fine. Something to keep in mind.

wenzeslaus · 2022-11-02T00:49:04Z

gui/wxpython/core/toolboxes.py

-    import __builtin__
+    if sys.version_info[0] == 2:
+        import __builtin__
+    elif locals().get("__builtin__", globals().get("__builtin__")) is None:
+        import builtins as __builtin__

    __builtin__._ = new_translator


Just drop the Python 2 part from here. The rest can be simpler, python/grass/init.py uses:

import builtins as _builtins _builtins.__dict__["_"] = _translate

I droped all Python 2 from that test now...

wenzeslaus · 2022-11-02T00:51:59Z

lib/init/testsuite/test_grass_tmp_mapset.py

@@ -30,13 +30,16 @@ class TestTmpMapset(unittest.TestCase):
    """Tests --tmp-mapset option of grass command"""

    # TODO: here we need a name of or path to the main GRASS GIS executable
-    executable = "grass"
+    executable = shutil.which("grass")


subprocess.run is just an interface to Popen like all the others. I don't expect it would help. shutil.which needs grass to be on path to work which is likely not fulfilled.

This test is now unique - we don't have other tests which test the executable - we we may have more in the future or even write such code, so perhaps ignoring it now and adding grass.script.setup.executable later is the way to go.

wenzeslaus · 2022-11-02T00:53:14Z

raster/r.mfilter/testsuite/test_r_mfilter.py

-        f = NamedTemporaryFile()
-        f.write(options)
-        f.flush()
-        return f
+        grass_tempfile = gs.tempfile(create=False)
+        with open(grass_tempfile, "wb") as f:
+            f.write(options)
+        return grass_tempfile


I still don't get what's wrong with NamedTemporaryFile.

I am a bit puzzled by that as well. NamedTemporaryFile leads to errors in the OSGeo4W tests. See:
https://github.com/OSGeo/grass/actions/runs/3368434482/jobs/5586972317#step:9:3031

I guess there is some cygwin-path-wrangling going on, maybe. At least the C:\\Users\\RUNNER~1 short form in the path string is not working. If the GRASS GIS DB is in the users home directory, the grass.tempfile may fail as well with the same issue... It is jus a try (as I am not coding on Windows)... If that fails too, we may revert and try to resolve the Path in a way that it is not shortened....

Putting together the NamedTemporaryFile doc and this SE, I think that we need to use delete=False, write to the file, close it, and then delete it manually.

wenzeslaus · 2022-11-02T00:54:48Z

scripts/g.extension/testsuite/test_addons_download.py

+                # Prebuild C-modules for MS Windows are unlikely to run
+                # on fresh compiled GRASS GIS


Oh, I missed what test_github_install_official is. That makes sense. Mention the different build versions in the comment.

ninsbl · 2022-11-02T07:51:29Z

subprocess.run is just an interface to Popen like all the others. I don't expect it would help. shutil.which needs grass to be on path to work which is likely not fulfilled.

This test is now unique - we don't have other tests which test the executable - we we may have more in the future or even write such code, so perhaps ignoring it now and adding grass.script.setup.executable later is the way to go.

I am fine with that.

wenzeslaus

I don't get the "does\\not\\exist" if ms_windows else "does/not/exist" changes. Windows supports / and our code should support it on Windows, too, so we should test for it (and fix it if needed). Adding additional tests for Windows for support of \ that would be different.

ninsbl · 2022-11-03T08:22:41Z

I don't get the "does\\not\\exist" if ms_windows else "does/not/exist" changes. Windows supports / and our code should support it on Windows, too, so we should test for it (and fix it if needed). Adding additional tests for Windows for support of \ that would be different.

Yes, but at least in the environment we run the tests in, paths are put together with OS specific separators, so tests fail with OSGeo4W

wenzeslaus · 2022-11-03T13:16:36Z

...paths are put together with OS specific separators, so tests fail with OSGeo4W

I'm just working from what is in the PR, but it seems to me that if tests fail in this case, the issue is in the actual code, not tests. The current tests need to work because we need to support / as a separator everywhere and we need to support paths with mixed separators (which is what likely happens in the current test cases). In any case, this looks like a material for a separate PR (and this PR is getting too long anyway).

ninsbl · 2022-11-03T22:12:14Z

I'm just working from what is in the PR, but it seems to me that if tests fail in this case, the issue is in the actual code, not tests. The current tests need to work because we need to support / as a separator everywhere and we need to support paths with mixed separators (which is what likely happens in the current test cases).

Path-handling with pathlib across plattforms is unfortunately still not trivial as some functions like resolve() behave differently on OSes, in certain cases e.g. if paths do not exist in the file system (see: https://discuss.python.org/t/pathlib-absolute-vs-resolve/2573). I guess that affects the tests too.
However, from my understanding, the main problem with the test_manage.py test is that we compare string representations of paths in some assertEqual functions to strings with universal separators (/). When transforming Path (which use /) to string objects, the pathlib uses platform-specific separators. So in str(Path().home()) the separator / is turned into \ on MS Windows. Thus a string representation of a WindowsPath will not be does/not/exist but does\not\exist. To fix this we could replace / with os.sep in the assertation as tried in the latest commit.

In any case, this looks like a material for a separate PR (and this PR is getting too long anyway).

Agreed. So I wrap up here. I am fine with reverting any change you suggest dropping for now... I can also split the PR...
And thanks for your patience and willingness to review repeatedly!

wenzeslaus · 2022-11-04T01:08:38Z

Path-handling with pathlib across platforms is unfortunately still not trivial as some functions like resolve() behave differently on OSes, in certain cases e.g. if paths do not exist in the file system...

I put a note about that to grass.grassdb.create.resolve_mapset_path and that's one place where GRASS GIS needs to deal with that. If the behavior is platform-dependent because of the libraries, so be it. We just need to document that and test on each platform what makes sense there.

...To fix this we could replace / with os.sep in the assertation as tried in the latest commit.

Fixing the assertions and just those which need that sounds great.

Agreed. So I wrap up here. I am fine with reverting any change you suggest dropping for now... I can also split the PR...

Smaller PRs are likely easier to review here. Generally, I think it is easier for people to approve or at least +1 thing related to one piece of code they know then >20 files. The *.in.lidar stuff could even go in without review given that it will be dropped in the future. You could also separate out anything which is controversial to get the other stuff in quickly.

And thanks for your patience and willingness to review repeatedly!

You are welcome. Ha ha, yes, this saturates all my current review time, but it is very important to get this fixed, so I'm happy to see that happening. Thank you!

wenzeslaus · 2022-11-04T01:46:49Z

lib/gis/testsuite/test_parser_json.py

-        stdout, stderr = subprocess.Popen(args, stdout=subprocess.PIPE).communicate()
+        stdout, stderr = subprocess.Popen(
+            args, stdout=subprocess.PIPE, shell=sys.platform == "win32"
+        ).communicate()


This would probably make more sense with direct support for this in grass.script, but to limit the solution only to here, would shutil.which work here at line 22?

wenzeslaus · 2022-11-04T01:52:26Z

vector/v.edit/testsuite/select_all_flag.sh

@@ -15,7 +16,7 @@ v.edit -r map=vedit_test tool=catadd layer=2 cats=10

 expected="10
 10"
-out=$(v.category option=print layer=2 input=vedit_test)
+out=$(v.category option=print layer=2 input=vedit_test| tr -d '\r')


Okay, please add comment that the \r part is for Windows, so we know when to remove it.

However, I think the role of Bash/sh/... tests is to provide option for "quick and dirty" tests and for very specific cases, so supporting only Linux or unix is fine. Something to keep in mind.

echoix · 2024-03-21T21:48:48Z

Do you still think this PR is the way forward? It has conflicts, but for instance the builtins import issue has need fixed in another PR in January, and there was a PR recently that changed a temporary file/NamedTemporaryFile, and it was the only usage in the code at the time.

ninsbl added 11 commits October 28, 2022 22:56

version dependent import of builtin

939e42a

test with python3

0a2f004

use shell in subprocess on Windows

9ecbb2d

find grass executable

edea1f9

handle OS newlines in testsuite

e2092cb

handle OS newlines in testsuite

c08635c

handle OS newlines in testsuite

fb075bf

handle OS newlines in testsuite

45bb9d7

compare file content not MD5

6de276a

ignore missing CRS info

50ddaad

remove windows newline from output

590cfd4

ninsbl added windows Microsoft Windows specific CI Continuous integration labels Oct 28, 2022

ninsbl added this to the 8.3.0 milestone Oct 28, 2022

ninsbl mentioned this pull request Oct 28, 2022

Address some MS Windows test failures #1683

Closed

ninsbl added 9 commits October 28, 2022 23:14

black

a7198dc

black

8951dc3

import sys

dd1d610

limit addons test on Win

31a2a3c

no proj check on Win

1cdc0fd

import shutil

4998aec

no proj check on Win

128d3ea

drop execution test on win32

39fc866

capture shell output on win32

7eb95cb

ninsbl added 3 commits October 31, 2022 23:50

use grass tempfile

74511ae

more shell output on windows

f89774f

black

1f96842

wenzeslaus reviewed Nov 1, 2022

View reviewed changes

gs and file object

01abed3

ninsbl added 4 commits November 1, 2022 08:40

type is str

0a0d009

define _

646d238

write bytes

6b72a10

import builtins

086cc65

wenzeslaus reviewed Nov 2, 2022

View reviewed changes

ninsbl added 2 commits November 2, 2022 08:30

remove Python 2

b219310

filter is path, flake8

9e7706f

ninsbl added 3 commits November 2, 2022 16:15

windows sep

5a3fd3e

do not try to close string

0114994

more os sep

5d4864e

wenzeslaus reviewed Nov 2, 2022

View reviewed changes

ninsbl added 2 commits November 3, 2022 22:55

os.sep in assertEqual for str(Path)

80fd808

black

db1473c

ninsbl changed the title ~~[WIP]: Address Windows specific issues in testsuite~~ Nov 3, 2022

wenzeslaus reviewed Nov 4, 2022

View reviewed changes

wenzeslaus modified the milestones: 8.3.0, 8.4.0 Feb 10, 2023

landam assigned ninsbl Nov 20, 2023

ninsbl mentioned this pull request Jan 4, 2024

[Bug] Windows CI Tests returns success status even for failed tests #3327

Open

wenzeslaus modified the milestones: 8.4.0, Future Apr 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: Address Windows specific issues in testsuite #2616

CI: Address Windows specific issues in testsuite #2616

ninsbl commented Oct 28, 2022

ninsbl commented Oct 31, 2022

wenzeslaus Nov 1, 2022

wenzeslaus Nov 1, 2022

ninsbl Nov 1, 2022

wenzeslaus Nov 1, 2022

ninsbl Nov 1, 2022

wenzeslaus Nov 2, 2022

wenzeslaus Nov 4, 2022

wenzeslaus Nov 1, 2022

wenzeslaus Nov 1, 2022

wenzeslaus Nov 1, 2022

wenzeslaus Nov 1, 2022

ninsbl Nov 1, 2022

wenzeslaus Nov 2, 2022

wenzeslaus Nov 1, 2022

ninsbl Nov 1, 2022

wenzeslaus Nov 4, 2022

wenzeslaus Nov 2, 2022

ninsbl Nov 2, 2022

wenzeslaus Nov 2, 2022

wenzeslaus Nov 2, 2022

ninsbl Nov 2, 2022

wenzeslaus Nov 4, 2022

wenzeslaus Nov 2, 2022

ninsbl commented Nov 2, 2022

wenzeslaus left a comment

ninsbl commented Nov 3, 2022

wenzeslaus commented Nov 3, 2022

ninsbl commented Nov 3, 2022

wenzeslaus commented Nov 4, 2022

wenzeslaus Nov 4, 2022

wenzeslaus Nov 4, 2022

echoix commented Mar 21, 2024

		python3 $GISBASE/gui/wxpython/core/toolboxes.py doctest
		python3 $GISBASE/gui/wxpython/core/toolboxes.py test

		@@ -1,4 +1,4 @@
		from tempfile import NamedTemporaryFile
		import grass.script as gscript

		# Prebuild C-modules for MS Windows are unlikely to run
		# on fresh compiled GRASS GIS

CI: Address Windows specific issues in testsuite #2616

Are you sure you want to change the base?

CI: Address Windows specific issues in testsuite #2616

Conversation

ninsbl commented Oct 28, 2022

ninsbl commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninsbl commented Nov 2, 2022

wenzeslaus left a comment

Choose a reason for hiding this comment

ninsbl commented Nov 3, 2022

wenzeslaus commented Nov 3, 2022

ninsbl commented Nov 3, 2022

wenzeslaus commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echoix commented Mar 21, 2024