Skip to content

fix(tests): decode configure_seed output as UTF-8 - #13278

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
bunbeans2-arch:fix/tests-configure-seed-utf8
Oct 30, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
bunbeans2-arch:fix/tests-configure-seed-utf8

Conversation

@bunbeans2-arch

Copy link
Copy Markdown
Contributor

What

tools/configure_seed.py reconfigures stdout and stderr to UTF-8, but tools/tests/test_configure_seed.py read them with text=True, which decodes with the locale encoding (cp1252 on a Western Windows install). When TEMP contains a non-ASCII character, test_unreadable_cache_is_reported_without_a_traceback and test_unwritable_seed_is_reported_without_a_traceback look for the path in stderr, find it as mojibake (e_é) and fail.

The six subprocess.run calls that start configure_seed now pass encoding="utf-8" instead of text=True, the same change #12993 makes for test_unwrap_self.py. The cmake calls in the round-trip tests are unchanged, since cmake does not write UTF-8 on its own. No tool code changes.

Tested

Windows 11 Pro 10.0.22631 (code page 1252), Python 3.13.5, MSYS2 UCRT64 GCC 16.1.0 and CMake on PATH. py -I tools/tests/test_configure_seed.py, whole file, 3 runs each:

TEMP and TMP main 1edecd6 this PR
...\e_é (inside code page 1252) 2 failures, 3 of 3 runs 29 tests OK, 3 of 3 runs
normal profile TEMP 29 tests OK 29 tests OK
...\zh_Ж (outside the code page) 2 failures + 2 errors 2 errors, the same as main

The two errors under Ж are setUpClass of ConfigureSeedTests and ConfigureSeedRoundTripTests: cmake -G Ninja with MinGW GCC fails to configure a project under that path, the toolchain limit already noted in #13083. This PR does not change them.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes (Claude Code)

🤖 Generated with Claude Code

configure_seed reconfigures stdout and stderr to UTF-8, but the tests
decoded them with the locale encoding. Under a TEMP path with a
non-ASCII character, the unreadable-cache and unwritable-seed tests
read the path back in cp1252 and failed on Windows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JJenkx

JJenkx commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Windows run of head 3ecb625ae1, merged onto main 1c6b316cf7 (clean merge, local only), compared with that main. Windows 11 Enterprise 10.0.22621 (ANSI code page 1252), Python 3.15.1 with -X utf8=0 and PYTHONUTF8=0 (3.15 defaults to UTF-8 mode, which would hide the locale decode), WinLibs GCC 15.2.0 UCRT r7, CMake 4.3.1, Ninja. python -m unittest tools/tests/test_configure_seed.py, with TEMP and TMP set to each directory:

TEMP main this PR
C:\x\tmp-ascii OK, 29 run, 2 skipped OK, 29 run, 2 skipped
C:\x\tëmp (inside code page 1252) 2 failures OK, 29 run, 2 skipped
C:\x\тест (outside the code page) 6 errors, 22 run 3 errors, 22 run

That matches your table. The three errors left under тест all come from real cmake -G Ninja configures (CalledProcessError from cmake -S ...\тест\..., plus a UnicodeDecodeError in the reader thread for the text=True CMake output). That's the MinGW path limit from #13083, not the configure_seed decoding this PR fixes. I see three rather than your two: the extra one is ChecksParsingTests.test_multiline_help_from_real_cmake_survives_seed_round_trip, which also configures a real CMake project under TEMP.

Logs: one run per cell; the 13278-py-<side>-<temp>.log files are on this machine.

AI-assisted: yes (Claude Code).

@malekaifa malekaifa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 3ecb625 on Windows ACP 1252. With TEMP=TMP=C:\tmp\e_é, the two affected ConfigureSeedPathTests ( est_unreadable_cache_is_reported_without_a_traceback and est_unwritable_seed_is_reported_without_a_traceback) both pass on this head. Running the same tests from current main under the same TEMP makes both fail because the parent process decodes configure_seed's UTF-8 stderr with the locale encoding, producing a mismatched/mojibake path. Current-main and #12614 pairwise merge trees are clean, and git diff --check passes. Hosted eligibility is green; generic relinker/ASan lanes are still queued/in progress. Non-blocking integration note: #12614 adds one new subprocess call that starts configure_seed with text=True; if #12614 lands after this PR, that call should use encoding='utf-8' as well so the locale-decoding gap is not reintroduced. No blocking issue found in #13278 itself.

@bunbeans2-arch

Copy link
Copy Markdown
Contributor Author

Thanks for the run. Re-ran under ...\zh_Ж with -v on current main 51c31ea and on this head, 3x each (Python 3.13.5, MSYS2 UCRT64 CMake 4.3.3 + GCC 16.1): here test_multiline_help_from_real_cmake_survives_seed_round_trip passes on both, so 2 errors remain on this PR (setUpClass of ConfigureSeedTests and ConfigureSeedRoundTripTests). Your third one from the WinLibs CMake run is the same cmake -G Ninja path limit under a non-code-page TEMP, so the count depends on the toolchain; this PR does not touch those cmake calls.

AI-assisted: yes (Claude Code)

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — looks good

The UTF-8 decoding change matches the tool’s explicit UTF-8 output, and the focused test file passes at the reviewed head.

What I ran

Reviewed head 3ecb625ae1cf6cdb7bc4e68871e842f3ae529ca0 against base e471817c68d4e1b1e44b0ce369ed88bafe4ae8ed; the diff is limited to subprocess output decoding in tools/tests/test_configure_seed.py. Environment: Debian 12 x86-64, Python 3.11.2, Git 2.39.5. No build was needed.

git diff --check e471817c68d4e1b1e44b0ce369ed88bafe4ae8ed...3ecb625ae1cf6cdb7bc4e68871e842f3ae529ca0
python3 tools/check_conventions.py --base e471817c68d4e1b1e44b0ce369ed88bafe4ae8ed --head 3ecb625ae1cf6cdb7bc4e68871e842f3ae529ca0 --title 'fix(tests): decode configure_seed output as UTF-8' --body /tmp/anyps5-pr-13278-body.md
python3 -m unittest tools.tests.test_configure_seed

Results

Check Result
Diff whitespace Passed.
Repository conventions Passed.
tools.tests.test_configure_seed Passed: 29 tests, 1 skipped.
Hosted checks Conventions, intake, labels, eligibility, Ubuntu relinker, Windows relinker, and relinker ASan passed. Two macOS relinker jobs remained queued at review time; Linux, Windows-specific, register-context, and write-watch jobs were skipped.
Unedited local output
All conventions checks passed.
.................s...........
----------------------------------------------------------------------
Ran 29 tests in 4.428s

OK (skipped=1)

Findings

No blocking or non-blocking findings in the reviewed test-only change. The tool reconfigures its output streams to UTF-8, and the changed subprocess calls now decode those streams using UTF-8.

Sources

Not verified

I did not run the tests on Windows, Linux CI, or macOS. The hosted macOS relinker jobs were queued; they do not exercise this Python test file.

@adriadam10 adriadam10 self-assigned this Oct 30, 2026
@adriadam10
adriadam10 merged commit 8ad062d into boykopovar:main Oct 30, 2026
13 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the six subprocess.run calls that start configure_seed: the tool reconfigures stdout/stderr to UTF-8 (tools/configure_seed.py:64-65), so decoding with encoding="utf-8" is correct; cmake calls are untouched. Windows ACP 1252 runs by @JJenkx and @malekaifa confirm both path tests fail on main and pass here; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants