Skip to content

test(profiles): make profile tests Windows-locale safe - #84024

Open
nanami7777777 wants to merge 2 commits into
NousResearch:mainfrom
nanami7777777:codex/fix-profile-tests-windows-locale
Open

test(profiles): make profile tests Windows-locale safe#84024
nanami7777777 wants to merge 2 commits into
NousResearch:mainfrom
nanami7777777:codex/fix-profile-tests-windows-locale

Conversation

@nanami7777777

Copy link
Copy Markdown
Contributor

Summary

  • use explicit UTF-8 for profile-test fixture file reads/writes so non-UTF-8 Windows locales do not decode seeded config/env files with the ANSI codepage
  • skip POSIX wrapper-name expectations on Windows and assert the .bat wrapper filename where list_profiles() reports alias paths
  • mark symlink-dependent profile tests with the existing require_symlinks marker so Windows hosts without Developer Mode/admin symlink rights skip cleanly

Fixes #83938.

Tests

  • scripts/run_tests.sh tests/hermes_cli/test_profiles.py
  • .venv/bin/python -m ruff check tests/hermes_cli/test_profiles.py

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage P3 Low — cosmetic, nice to have sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 11, 2026

@monerostar monerostar 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.

Native Win11 verification (monerostar)

Could not leave a formal Approve (fork collaborator scope) — review-as-comment with live evidence.

Host: physical Windows 11 build 26200, CPython 3.11.15. This box is a non-UTF-8-locale-safe target (ACP/system ANSI is not guaranteed UTF-8) and does not have Developer Mode, so require_symlinks correctly skips.

SHA reviewed: 19e9bd0044329cbf30329cb869c6d03d73e5fe1b

What this PR gets right

The three stated Windows problems are real and the patch matches existing repo patterns:

  • explicit encoding="utf-8" on fixture reads/writes
  • .bat alias filename on win32 (qiaobusi.bat vs qiaobusi)
  • @pytest.mark.require_symlinks on the two symlink tests, plus skipif(win32) on the POSIX wrapper script

Live results

pytest tests/hermes_cli/test_profiles.py -o addopts=
→ 2 failed, 45 passed, 3 skipped in 2.36s

Targeted slice (alias_path or wrapper or encoding or symlink or bat):

4 passed, 3 skipped
SKIPPED POSIX wrapper script
SKIPPED require_symlinks ×2 (no admin/Developer Mode)

The .bat alias assertion passed on this host.

Remaining native-Windows failures (pre-existing, not introduced here)

Both red tests are the classic NTFS os.chmod(..., 0o600) no-op (#56923):

FAILED TestCreateProfile::test_seeds_placeholder_env_file
FAILED TestBackfillProfileEnvs::test_copies_default_env_into_envless_profiles
  assert 438 == 384   # S_IMODE(0o100666) vs 0o600

chmod 0o600 does not change Win32 mode bits, so st_mode & 0o777 stays 0o666 (438). This PR does not touch those asserts. CI stays green because Linux runners enforce POSIX modes.

Non-blocking suggestion: guard the two == 0o600 checks with sys.platform != "win32" (same pattern as #56946), or this file will keep failing for native Windows contributors even after the locale/symlink fixes.

CI on the PR is green (All required checks pass). The Windows-contributor path is improved; the chmod asserts are the leftover red.

@nanami7777777

Copy link
Copy Markdown
Contributor Author

Thanks for the native Win11 verification and the detailed split between PR-caused and pre-existing failures. I pushed f6e597c to guard the two POSIX mode-bit assertions on win32 while keeping the file existence/content checks intact.

Validated locally:

  • scripts/run_tests.sh tests/hermes_cli/test_profiles.py (48 passed, 2 skipped on darwin)
  • .venv/bin/ruff check tests/hermes_cli/test_profiles.py
  • git diff --check

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

test(profiles): make profile tests Windows-locale safe

  1. test_seeds_placeholder_env_file and test_copies_default_env_into_envless_profiles now skip the 0o600 assertion on win32 but still run — on Windows they effectively assert nothing about file permissions while reporting pass. A pytest.skip with a reason (or splitting the permission assertion into a @pytest.mark.posix_only test) would make the gap explicit; as written, a future permissions regression on Windows is invisible.
  2. @pytest.mark.require_symlinks is used on the two symlink tests — verified the marker is registered in tests/conftest.py, so no strict-marking surprise. Good.
  3. test_list_profiles_surfaces_custom_alias asserting qiaobusi.bat on win32 correctly pins the Windows wrapper naming; worth confirming create_wrapper_script is what actually guarantees .bat on Windows (the test now documents that contract on both platforms).
  4. The explicit encoding="utf-8" pass is the right fix for locale-dependent default encodings; one nit — write_text(..., encoding="utf-8") combined with the \r\n literal in the .bat fixture keeps CRLF explicit on every host, which is correct but means a POSIX host writes CRLF content into a file it will never execute. Harmless, just noting.

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

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: tests/hermes_cli/test_profiles.py: 8 failures on Windows / non-UTF-8 locale

4 participants