Fix local unit tests #11883

bruchar1 · 2023-06-15T11:12:11Z

On my Windows machine, a lot of unit tests are failing. This contains fixes to make all them pass.

Most of the fixes are related to the encoding used by the windows console.

I'm still not sure if locale.getpreferredencoding() is the way to go... On my machine, getpreferredencoding() is always cp1252, stdout.encoding is utf8, and the actual console codepage is given by windll.kernel32.GetConsoleOutputCP()...
Maybe should I use the API call on Windows instead of getpreferredencoding?

mesonbuild/interpreter/interpreter.py

unittests/allplatformstests.py

eli-schwartz · 2023-06-15T12:29:40Z

unittests/allplatformstests.py

@@ -3711,7 +3711,7 @@ def test_commands_documented(self):
        ## Validate commands

        md_commands = {k for k,v in md_command_sections.items()}
-        help_output = self._run(self.meson_command + ['--help'])
+        help_output = self._run(self.meson_command + ['--help'], stderr=False)


What warnings are we encountering???

The warning about missing encoding parameter in platform.py on Python 3.11. (python/cpython#100750)

Perhaps the correct solution is to extend

meson/mesonbuild/mesonmain.py

Lines 246 to 256 in f1a58a3

if sys.version_info >= (3, 10) and os.environ.get('MESON_RUNNING_IN_PROJECT_TESTS'):

# workaround for https://bugs.python.org/issue34624

import warnings

warnings.filterwarnings('error', category=EncodingWarning, module='mesonbuild')

# python 3.11 adds a warning that in 3.15, UTF-8 mode will be default.

# This is fantastic news, we'd love that. Less fantastic: this warning is silly,

# we *want* these checks to be affected. Plus, the recommended alternative API

# would (in addition to warning people when UTF-8 mode removed the problem) also

# require using a minimum python version of 3.11 (in which the warning was added)

# or add verbose if/else soup.

warnings.filterwarnings('ignore', message="UTF-8 Mode affects .*getpreferredencoding", category=EncodingWarning)

Using PYTHONWARNINGS env var seems to mitigate the problem

unittests/allplatformstests.py

eli-schwartz · 2023-06-15T13:31:15Z

mesonbuild/utils/universal.py

+if sys.version_info >= (3, 11):
+    getencoding = locale.getencoding
+else:
+    getencoding = locale.getpreferredencoding


This upstream python warning is garbage, I seem to recall I filtered it out explicitly to prevent errors in the testsuite. I don't care if it collects warnings in the testsuite. Warnings are not errors, especially when the warnings are fallaciously incorrect.

unittests/rewritetests.py

eli-schwartz · 2023-06-15T13:33:19Z

unittests/internaltests.py

@@ -1648,7 +1648,7 @@ def test_interpreter_unpicklable(self) -> None:
        build = mock.Mock()
        build.environment = mock.Mock()
        build.environment.get_source_dir = mock.Mock(return_value='')
-        with mock.patch('mesonbuild.interpreter.Interpreter._redetect_machines', mock.Mock()), \
+        with mock.patch('mesonbuild.interpreter.interpreter.Interpreter._redetect_machines', mock.Mock()), \


Can you give more details about this module name clash?

AttributeError: <module 'mesonbuild.interpreter.Interpreter' from 'C:\\env\\meson\\mesonbuild\\interpreter\\Interpreter.py'> does not have the attribute '_redetect_machines'

It tries to load _redetect_machines from interpreter.py module instead of Interpreter class. Not sure why it does like this on my machine and not in CI...

<module 'mesonbuild.interpreter.Interpreter' from 'C:\\env\\meson\\mesonbuild\\interpreter\\Interpreter.py'>

This seems extremely wrong and will not be fixed by this change, probably.

mesonbuild.interpreter is a package -- it has an __init__.py which imports from .interpreter import Interpreter

mesonbuild.interpreter.interpreter is a module, interpreter.py, while mesonbuild.interpreter.Interpreter is a class object

What I don't understand is -- when python sees mesonbuild.interpreter.Interpreter, it should first load mesonbuild/interpreter/__init__.py, then evaluate the scope of such, and have mesonbuild.interpreter.Interpreter as a symbol manually defined at package scope. So it should not be trying to load it as a file instead.

I investigated further. It seems the bug only occurs when meson is in editable install in my venv. With a normal install, or without the venv, the test passes. I still don't understand, but I guess I can drop this commit for now.

eli-schwartz · 2023-06-15T13:34:35Z

mesonbuild/utils/vsenv.py

@@ -92,7 +92,7 @@ def _setup_vsenv(force: bool) -> bool:
    bat_file.write(bat_contents)
    bat_file.flush()
    bat_file.close()
-    bat_output = subprocess.check_output(bat_file.name, universal_newlines=True)
+    bat_output = subprocess.check_output(bat_file.name, universal_newlines=True, encoding=getencoding())


Commit message is wrong since this has nothing to do with unittests. It's also a duplicate of an existing PR.

commit squashed with other encoding related commits. It is related to unittests because otherwise WindowsTests.test_vsenv_option fails on my machine. Not sure why it is ok on CI: probably because it uses a different default locale.
It the existing PR, I used utf-8 as encoding, but getencoding() is probably better...

eli-schwartz · 2023-06-15T13:35:30Z

mesonbuild/mtest.py

@@ -525,7 +525,7 @@ def __init__(self) -> None:
        self.sub = self.RTRI
        self.spinner = self.SPINNER
        try:
-            self.output_start.encode(sys.stdout.encoding or 'ascii')
+            self.output_start.encode(getencoding() or 'ascii')


What is this fixing exactly?

On my machine, getencoding() gives cp1252, and sys.stdout.encoding gives utf-8. When a unittest run meson in a subprocess, it uses cp1252, but test outputs in utf-8. Therefore, it causes a UnicodeDecodeError on the RTRI char used as prefix for subtests.

eli-schwartz · 2023-06-15T13:38:25Z

mesonbuild/utils/universal.py

+        from ctypes import windll
+
+        try:
+            # This seems the only reliable way to get current console encoding


Other than the fact that getpreferredencoding bad, and you spent a few commit messages adding something that apparently doesn't even work and then backing it out....what was wrong with the way we were doing this before this PR?

Previous attempt worked, as it allowed the unittests to pass both on my machine and on CI. But it prevented using utf-8 when the console allowed it, since it used cp1252 instead of cp65001, even if I called chcp 65001. Console encoding on Windows is a big mess, and I'm still not 100% sure what is the right approach...

eli-schwartz · 2023-06-16T06:14:42Z

I accepted a couple of these patches on their own:

unittests: ensure terminal has 80 chars width
unittests: skip test using symlink if not available
unittests: fix test_scripts_loaded_modules matching exactly our own modules (with tweaked commit message)

The remaining commits in this PR are the ones I'm less sure of, in particular they include the change that is currently making CI fail. ;)

bruchar1 · 2023-06-16T13:33:51Z

The remaining commits in this PR are the ones I'm less sure of, in particular they include the change that is currently making CI fail. ;)

CI is all-green now! (managing encodings in Python is a big mess...)

tristan957 · 2023-06-20T20:32:36Z

@eli-schwartz is this one good to go now?

eli-schwartz · 2023-06-20T20:41:32Z

unittests/internaltests.py

@@ -1648,7 +1648,7 @@ def test_interpreter_unpicklable(self) -> None:
        build = mock.Mock()
        build.environment = mock.Mock()
        build.environment.get_source_dir = mock.Mock(return_value='')
-        with mock.patch('mesonbuild.interpreter.Interpreter._redetect_machines', mock.Mock()), \
+        with mock.patch('mesonbuild.interpreter.interpreter.Interpreter._redetect_machines', mock.Mock()), \


<module 'mesonbuild.interpreter.Interpreter' from 'C:\\env\\meson\\mesonbuild\\interpreter\\Interpreter.py'>

This seems extremely wrong and will not be fixed by this change, probably.

mesonbuild.interpreter is a package -- it has an __init__.py which imports from .interpreter import Interpreter

mesonbuild.interpreter.interpreter is a module, interpreter.py, while mesonbuild.interpreter.Interpreter is a class object

What I don't understand is -- when python sees mesonbuild.interpreter.Interpreter, it should first load mesonbuild/interpreter/__init__.py, then evaluate the scope of such, and have mesonbuild.interpreter.Interpreter as a symbol manually defined at package scope. So it should not be trying to load it as a file instead.

eli-schwartz · 2023-06-20T20:47:05Z

unittests/allplatformstests.py

@@ -3677,7 +3677,7 @@ def test_link_language_linker(self):
        self.init(testdir)

        build_ninja = os.path.join(self.builddir, 'build.ninja')
-        with open(build_ninja, encoding='utf-8') as f:
+        with open(build_ninja, encoding=getencoding()) as f:


This is definitely wrong -- we write it as utf-8, hence why we open it like that, and in general ninja expects:

Ninja is mostly encoding agnostic, as long as the bytes Ninja cares about (like slashes in paths) are ASCII. This means e.g. UTF-8 or ISO-8859-1 input files ought to work.

Allowing any codepage even if it's not ASCII-compatible at all seems like an error.

I changed for reading as bytes.

eli-schwartz · 2023-06-20T20:53:17Z

unittests/baseplatformtests.py

@@ -255,7 +255,7 @@ def init(self, srcdir, *,
                    self._print_meson_log()
                    raise
                out = self._get_meson_log()  # best we can do here
-        return out
+        return out or ''


This seems random, what's it trying to do?

get_meson_log() can return None, but the function must return a str. If an exception occurs and there is no log to read, the function returns None, and it causes another exception when the test performs a test on the out value.

eli-schwartz · 2023-06-20T20:57:17Z

@eli-schwartz is this one good to go now?

?

Like I said, I'm generally suspicious that I don't understand the necessity of the locale changes, at least.

Fixes mesonbuild#11770 Use console codepage, on Windows On Windows, locale.getpreferredencoding() or sys.getdefaultencoding() do not reflect the codepage from the console. It seems the only way to get the information is to use the Windows API. https://stackoverflow.com/a/66137691/16234629

bruchar1 · 2023-06-21T02:18:11Z

Like I said, I'm generally suspicious that I don't understand the necessity of the locale changes, at least.

On posix systems, I guess the default locale encoding is almost always utf-8. Alas, this it not the case on Windows. Unless you explicitly set the console encoding to utf-8 (i.e. chcp 65001), most of the tests are currently failing because of encoding issues.

When not explicitly set, the encoding of most operations fall back to the locale prefered encoding. Therefore, on posix systems, the changes I did only explicitly set what was implicitly done.

However, this doesn't seem reliable on Windows, where the locale prefered encoding does not reflect the console chcp. With the proposed changes, tests are now working regardless of the console encoding, and it probably address some encoding errors when using meson, like mentioned in #11770.

This prevents errors when sys.executable contains spaces

bruchar1 requested a review from jpakkane as a code owner June 15, 2023 11:12

bruchar1 force-pushed the fix-local-unit-tests branch 2 times, most recently from 598e494 to 61a6634 Compare June 15, 2023 13:21

eli-schwartz requested changes Jun 15, 2023

View reviewed changes

bruchar1 force-pushed the fix-local-unit-tests branch 4 times, most recently from 8da8df0 to 19e3f3b Compare June 16, 2023 01:28

bruchar1 force-pushed the fix-local-unit-tests branch 3 times, most recently from 40a60db to 3eb3770 Compare June 16, 2023 11:46

eli-schwartz reviewed Jun 20, 2023

View reviewed changes

bruchar1 added 2 commits June 20, 2023 21:28

unittests: remove warnings from cmd output

a3eaef6

bruchar1 force-pushed the fix-local-unit-tests branch from 3eb3770 to bab20d9 Compare June 21, 2023 02:12

unittests: quote sys.executable in NatifeFileTests

128eecb

This prevents errors when sys.executable contains spaces

bruchar1 mentioned this pull request Aug 8, 2023

tests: some Windows fixes with newer Python #12088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix local unit tests #11883

Fix local unit tests #11883

bruchar1 commented Jun 15, 2023 •

edited

Loading

eli-schwartz Jun 15, 2023

bruchar1 Jun 15, 2023

eli-schwartz Jun 15, 2023

bruchar1 Jun 15, 2023

eli-schwartz Jun 15, 2023

eli-schwartz Jun 15, 2023

bruchar1 Jun 15, 2023

eli-schwartz Jun 20, 2023

bruchar1 Jun 21, 2023

eli-schwartz Jun 15, 2023

bruchar1 Jun 15, 2023 •

edited

Loading

eli-schwartz Jun 15, 2023

bruchar1 Jun 15, 2023

eli-schwartz Jun 15, 2023

bruchar1 Jun 15, 2023

eli-schwartz commented Jun 16, 2023

bruchar1 commented Jun 16, 2023

tristan957 commented Jun 20, 2023

eli-schwartz Jun 20, 2023

eli-schwartz Jun 20, 2023

bruchar1 Jun 21, 2023

eli-schwartz Jun 20, 2023

bruchar1 Jun 21, 2023

eli-schwartz commented Jun 20, 2023

bruchar1 commented Jun 21, 2023 •

edited

Loading

	if sys.version_info >= (3, 10) and os.environ.get('MESON_RUNNING_IN_PROJECT_TESTS'):
	# workaround for https://bugs.python.org/issue34624
	import warnings
	warnings.filterwarnings('error', category=EncodingWarning, module='mesonbuild')
	# python 3.11 adds a warning that in 3.15, UTF-8 mode will be default.
	# This is fantastic news, we'd love that. Less fantastic: this warning is silly,
	# we want these checks to be affected. Plus, the recommended alternative API
	# would (in addition to warning people when UTF-8 mode removed the problem) also
	# require using a minimum python version of 3.11 (in which the warning was added)
	# or add verbose if/else soup.
	warnings.filterwarnings('ignore', message="UTF-8 Mode affects .*getpreferredencoding", category=EncodingWarning)

Fix local unit tests #11883

Are you sure you want to change the base?

Fix local unit tests #11883

Conversation

bruchar1 commented Jun 15, 2023 • edited Loading

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

bruchar1 Jun 15, 2023 • edited Loading

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

eli-schwartz commented Jun 16, 2023

bruchar1 commented Jun 16, 2023

tristan957 commented Jun 20, 2023

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

eli-schwartz commented Jun 20, 2023

bruchar1 commented Jun 21, 2023 • edited Loading

bruchar1 commented Jun 15, 2023 •

edited

Loading

bruchar1 Jun 15, 2023 •

edited

Loading

bruchar1 commented Jun 21, 2023 •

edited

Loading