Skip to content

test(tools): support Python 3.10 in encoding regressions - #4361

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
Roojool:contrib/round2-c20
Oct 12, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
Roojool:contrib/round2-c20

Conversation

@Roojool

@Roojool Roojool commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

What

pr_overlap_encoding errors on Python 3.10 before testing any Unicode handling: its command_output context manager patches subprocess._text_encoding, which that interpreter does not provide. A native Windows run on main produced eight AttributeError errors. The same limitation is recorded in the validation results of #3986, #1764, #1786 and #2391.

Keep the existing CP1252 simulation on newer interpreters. On Python 3.10, intercept the io.TextIOWrapper locale encoding that Popen actually uses. Explicit UTF-8 arguments still pass through unchanged. A real subprocess test checks the exact CP1252-decoded output when no encoding is supplied and exact Unicode output when UTF-8 is supplied. No nonexistent mock attribute is created.

Only tools/tests/test_pr_overlap_encoding.py changes. Production UTF-8 handling from the previous fixes remains unchanged; this repairs the regression tests rather than reimplementing those fixes. No dependency added. Authored by Rujul Talekar with Muse Spark 1.3 and Codex assistance. This is developer test-suite compatibility, with no title or new CI-failure claim.

Tested

Baseline: 2ca6198e9a43424c2417f2c4c000ecd3d141811a. Rebased onto main 436e310ddb56fcf5dd1d40ba1f8113fe8cb8c113 and repeated final validation.

  • Native Windows, Python 3.10.11: unchanged baseline python -m unittest tools.tests.test_pr_overlap_encoding -v fails with eight errors, AttributeError: module 'subprocess' has no attribute '_text_encoding'. With the fix, 11/11 pass.
  • Linux via WSL2 Ubuntu, Python 3.14.4: the same fixed encoding suite 11/11 passes. The baseline failure concerns Python 3.10; no Linux baseline failure is claimed.
  • Final neighboring validation on both systems, 29/29 pass: python -m unittest tools.tests.test_pr_overlap tools.tests.test_pr_overlap_dependencies tools.tests.test_pr_overlap_encoding tools.tests.test_pr_overlap_exports -v (python3 on Linux).
  • In a disposable copy outside the repository, removing production's explicit UTF-8 arguments makes the simulation fail on both platforms: Linux raises UnicodeDecodeError; Windows's reader thread raises the decode error and the caller rejects the missing output. Unmodified production decodes exactly on both. The source branch's production file was never altered.
  • python tools/check_conventions.py --base upstream/main --head HEAD: passed. git diff --check: passed.
  • No full C++ rebuild or title/GPU runs; this changes one Python test file. Tested interpreter versions are 3.10 and 3.14; older interpreters were not checked.

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 in TechnicalDebt
  • 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
  • AI-assisted: yes

…rlap tests

command_output patched subprocess._text_encoding, which does not exist on Python 3.10, so all eight tests errored before their assertions. Use subprocess._text_encoding where available and map io.TextIOWrapper locale to cp1252 on older interpreters, so a subprocess with no explicit encoding decodes as cp1252 on both. Add a behavioral check that default decodes as cp1252 while explicit utf-8 stays correct.

Authored by Rujul Talekar, with Muse Spark 1.3 assistance.
Copilot AI balanced review requested due to automatic review settings October 12, 2026 07:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@MarcZX10

Copy link
Copy Markdown
Contributor

I ran the focused overlap encoding and neighboring regression tests locally on Linux/Python 3.11: 29/29 passed. The conventions check and git diff --check also pass. This exercises Python 3.11’s _text_encoding branch; I did not run Python 3.10 locally. The CPython 3.10 subprocess.Popen implementation sets the default encoding to "locale" before constructing its TextIOWrapper, which matches the fallback branch added here.

@Zurisen

Zurisen commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Native Windows run of e991645c merged into main 2a5eca11 (Windows 11 Home 26200), with Python 3.10.11 and 3.12.10:

  • tools/tests/test_pr_overlap_encoding.py: 11/11 on both (on main, 3.10 errors in every case with AttributeError: ... '_text_encoding').
  • Guard check: with encoding="utf-8" removed from pr_overlap.gh, it fails on both (test_gh_json_preserves_unicode errors, test_gh_error_preserves_unicode fails), so the regression stays covered on 3.10.

A smaller variant, if you want one, with no version branch or io.TextIOWrapper patch: the fake subprocess.run in command_output can give a text-mode call without an encoding cp1252 itself, which is what a cp1252 default locale does on every version:

def run(command, **kwargs):
    if kwargs.get("text") or kwargs.get("universal_newlines"):
        kwargs.setdefault("encoding", "cp1252")
    return RUN([sys.executable, "-c", code], **kwargs)

With that in place of the _text_encoding patch, the same 10 cases pass on 3.10 and 3.12 and fail the same way without encoding= (commit 2ba0da75 on Zurisen:fix/pr-overlap-test-python310). Your version is fine as is; this is only if a reviewer prefers fewer moving parts.

@oneandonlydean
oneandonlydean merged commit ee2c0f9 into boykopovar:main Oct 12, 2026
14 checks passed
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