CI: Address Windows specific issues in testsuite by ninsbl · Pull Request #2616 · OSGeo/grass

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

Replaces: #1683

July 2026 AI analysis of what is possibly still relevant:

~~r.mfilter: the NamedTemporaryFile → gs.tempfile fix is a textbook Windows bug (can't reopen an open NamedTemporaryFile) and directly explains the 6 xfail'd tests there.~~ r.mfilter: Avoid reopening NamedTemporaryFile on Windows in testsuite #7878
~~grassdb/test_manage.py: the os.sep-aware assertions map one-to-one onto the 4 xfail'd tests.~~ grass.grassdb: Fix path separator expectations in manage tests on Windows #8050
test_parser_json.py: uses raw subprocess.Popen (not grass.script), so the shell= on win32 fix is still relevant; all 3 tests are xfail'd.
~~test_checkers.py / test_assertions.py: the newline="" fixes for MD5-checked files still apply, though test_assertions.py now writes via Path.write_text(), so the fix needs rewriting.~~ grass.gunittest: Preserve exact bytes in MD5 test fixtures #8039
case.py: normalizing/stripping the reference string in assertLooksLike/assertModuleKeyValue (main only normalizes actual).
toolboxes.sh (python→python3) and v.edit/select_all_flag.sh (tr -d '\r'): both files are still on the exclusion list in .github/workflows/osgeo4w_gunittest.cfg, so the fixes are still applicable.

These would be better as individual PRs.

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

+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

-    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


    # 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

+    # 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

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


import grass.script as gs

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

    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

+                # 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

 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-04T01:46:49Z

-        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

 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.

echoix · 2024-09-21T19:29:37Z

I've skimmed through very quickly, and I think cutting out some line endings in the tests only for Windows is maybe not the right approach. It is something that we should probably fix for real instead, and to not care too much about io with Python, as it is handled way more transparently than on Python 2

echoix · 2025-10-22T02:21:55Z

Is there still something useful and relevant here that should be kept, 3 years later? Our testing situation is a bit better now, our Python code quality is way up since more than a year and a half, grass start up and initialization, especially for usage as a Python library is recently better, etc.

petrasovaa · 2026-07-17T20:30:06Z

AI analysis of this PR, what is possibly still relevant:

r.mfilter: the NamedTemporaryFile → gs.tempfile fix is a textbook Windows bug (can't reopen an open NamedTemporaryFile) and directly explains the 6 xfail'd tests there.
grassdb/test_manage.py: the os.sep-aware assertions map one-to-one onto the 4 xfail'd tests.
test_parser_json.py: uses raw subprocess.Popen (not grass.script), so the shell= on win32 fix is still relevant; all 3 tests are xfail'd.
test_checkers.py / test_assertions.py: the newline="" fixes for MD5-checked files still apply, though test_assertions.py now writes via Path.write_text(), so the fix needs rewriting.
case.py: normalizing/stripping the reference string in assertLooksLike/assertModuleKeyValue (main only normalizes actual).
toolboxes.sh (python→python3) and v.edit/select_all_flag.sh (tr -d '\r'): both files are still on the exclusion list in .github/workflows/osgeo4w_gunittest.cfg, so the fixes are still applicable.

These would be better as individual PRs.

ninsbl · 2026-07-17T23:19:55Z

AI analysis of this PR, what is possibly still relevant:

t.rast.import / t.vect.import is fixed in t.rast.import: use platform-indendent tempdir default #7742

We can keep the PR open for reference or open an issue, close this PR and point to the PR in the issue... Both is fine with me. I agree it should be implemented in separate, smaller PRs...

@ninsbl

On Windows, a file opened via tempfile.NamedTemporaryFile() cannot be reopened by another process while the Python handle is still open, so r.mfilter failed to read the filter file created this way. Write the filter contents to a path from grass.script.tempfile() instead, which does not hold the file open, and drop the @xfail_windows markers this fix should resolve. Extracted and reworked as a standalone change from PR OSGeo#2616 (by Stefan Blumentrath / @ninsbl), which combined this fix with many other Windows-specific testsuite changes and has been open, unrebased, since 2022. Re-derived against current main rather than cherry-picked, because @ninsbl's branch predates unrelated fixes already merged to this same test file (a docstring/variance correction and other @xfail_windows additions), which a direct cherry-pick would have clobbered. This has not been verified on an actual Windows run; CI on this PR should confirm whether the @xfail_windows markers can indeed be removed. Analysis of PR OSGeo#2616 assisted by AI (Claude), including identifying this as an easily isolated, self-contained candidate for its own PR; verify before merging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HjAoKyq8bFN7cnNaZkwtV6

@ninsbl

On Windows, a file opened via tempfile.NamedTemporaryFile() cannot be reopened by another process while the Python handle is still open, so r.mfilter failed to read the filter file created this way. Write the filter contents to a path from grass.script.tempfile() instead, which does not hold the file open, and drop the @xfail_windows markers this fix should resolve. Extracted and reworked as a standalone change from PR OSGeo#2616 (by Stefan Blumentrath / @ninsbl), which combined this fix with many other Windows-specific testsuite changes and has been open, unrebased, since 2022. Re-derived against current main rather than cherry-picked, because @ninsbl's branch predates unrelated fixes already merged to this same test file (a docstring/variance correction and other @xfail_windows additions), which a direct cherry-pick would have clobbered. This has not been verified on an actual Windows run; CI on this PR should confirm whether the @xfail_windows markers can indeed be removed. Analysis of PR OSGeo#2616 assisted by AI (Claude), including identifying this as an easily isolated, self-contained candidate for its own PR; verify before merging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Python's default text-mode writing translates "\n" to os.linesep, which on Windows turns the MD5 test fixtures into "\r\n" files. Two tests hash them in binary mode against MD5 sums computed from "\n" content: test_text_file_binary in test_checkers.py and test_assertFileMd5 in test_assertions.py. Write these fixtures with newline="" so the bytes match the hardcoded sums on every platform. Do the same for the other fixtures written next to file_with_md5, since test_assertFilesEqualMd5 compares their hashes against it. Remove the now obsolete @xfail_windows markers from the two tests, because an unexpected success makes the gunittest run fail. Also fix the "platfrom" typos in the test_text_file_platform test name and the platform_newline attribute. Based on the newline handling in OSGeo#2616 by Stefan Blumentrath, reworked against current main with AI assistance. Co-authored-by: Stefan Blumentrath <4246620+ninsbl@users.noreply.github.com>

Python's default text-mode writing translates "\n" to os.linesep, which on Windows turns the MD5 test fixtures into "\r\n" files. Two tests hash them in binary mode against MD5 sums computed from "\n" content: test_text_file_binary in test_checkers.py and test_assertFileMd5 in test_assertions.py. Write these fixtures with newline="" so the bytes match the hardcoded sums on every platform. Do the same for the other fixtures written next to file_with_md5, since test_assertFilesEqualMd5 compares their hashes against it. Remove the now obsolete @xfail_windows markers from the two tests, because an unexpected success makes the gunittest run fail. Also fix the "platfrom" typos in the test_text_file_platform test name and the platform_newline attribute. Based on the newline handling in #2616 by Stefan Blumentrath, reworked against current main with AI assistance. Co-authored-by: Stefan Blumentrath <4246620+ninsbl@users.noreply.github.com>

…dows pathlib uses os.sep in the string form of paths, so the tests that compare against forward-slash strings failed on Windows. Compare against the same strings with os.sep and drop the xfail_windows markers from the four tests which this fixes. Extracted from OSGeo#2616 by Stefan Blumentrath.

…dows (#8050) pathlib uses os.sep in the string form of paths, so the tests that compare against forward-slash strings failed on Windows. Compare against the same strings with os.sep and drop the xfail_windows markers from the four tests which this fixes. Extracted from #2616 by Stefan Blumentrath. Co-authored-by: Stefan Blumentrath <stefan.blumentrath@gmx.de>

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

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

ninsbl mentioned this pull request Sep 21, 2024

tests: Re-enable passing tests from .gunittest.cfg exclusion list #4358

Merged

echoix mentioned this pull request Sep 22, 2024

tests: Add xfail_windows decorator for failing tests on Windows #4362

Merged

echoix added the conflicts/needs rebase Rebase to or merge with the latest base branch is needed label Oct 22, 2025

echoix mentioned this pull request Sep 2, 2026

r.mfilter: Avoid reopening NamedTemporaryFile on Windows in testsuite echoix/grass#677

Closed

4 of 8 tasks

This was referenced Sep 2, 2026

r.mfilter: Avoid reopening NamedTemporaryFile on Windows in testsuite #7878

Merged

grass.gunittest: Preserve exact bytes in MD5 test fixtures echoix/grass#680

Closed

This was referenced Oct 9, 2026

grass.gunittest: Preserve exact bytes in MD5 test fixtures echoix/grass#730

Closed

grass.gunittest: Preserve exact bytes in MD5 test fixtures #8039

Merged

This was referenced Oct 11, 2026

grass.grassdb: Fix path separator expectations in manage tests on Windows echoix/grass#737

Closed

grass.grassdb: Fix path separator expectations in manage tests on Windows #8050

Merged

		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

Conversation

ninsbl commented Oct 28, 2022 • edited by echoix Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ninsbl commented Oct 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echoix commented Mar 21, 2024

Uh oh!

echoix commented Sep 21, 2024

Uh oh!

echoix commented Oct 22, 2025

Uh oh!

petrasovaa commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ninsbl commented Jul 17, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ninsbl commented Oct 28, 2022 •

edited by echoix

Loading

petrasovaa commented Jul 17, 2026 •

edited

Loading