Repository navigation
fix: make test suite and tools portable across default encodings and python -OO (#484) - #540
Conversation
rajnisht7
left a comment
There was a problem hiding this comment.
Encoding fix looks good, fixing encoding in several conformance/tests/ files (conftest, anchor/manifest-ref vectors, measurement harness, unit loader/runner/CLI, generator). The new CI step is:
LC_ALL=C PYTHONUTF8=0 pytest
PYTHONOPTIMIZE=2 pytest tests/test_review_bar.pyRoot pyproject.toml has testpaths = ["tests"], so that only runs root tests/. conformance-ci.yml still runs under a normal UTF-8 locale. So the conformance portability fixes aren’t regression-protected.
kindly add a non-UTF-8 run for conformance as well (in ci.yml or conformance-ci.yml), e.g. something that actually executes conformance/tests under LC_ALL=C PYTHONUTF8=0.
Also minor:
PYTHONOPTIMIZE=2only exercises test_review_bar.py. tools/check_dashes.py was also fixed for -OO but isn’t run under -OO in CI, worth adding PYTHONOPTIMIZE=2 python tools/check_dashes.py.- In
review_bar.py, preferdescription=(__doc__ or "").split("\n\n")[0]so the-OOguard doesn’t change the existing “first paragraph” split behavior.
imran-siddique
left a comment
There was a problem hiding this comment.
Approving 68b778c. This is the scope agreed on #484. The red test (3.11) and (3.12) on 8803282 was the new LC_ALL=C job doing its job on the non-ASCII filename test, and 68b778c skips that one case. On Windows cp1252 the git-quoted path test fails on #537, #538 and #539 heads and passes here. python -OO tools/check_dashes.py --help and tools/review_bar.py --help crash with AttributeError on 2bbecec and work here. Ruff and check_dashes are clean.
dinakarjs
left a comment
There was a problem hiding this comment.
Independent bounded portability review at 68b778c. No blocking finding in the review-bar/tool-help boundaries tested here.
Fresh Python 3.12.14 results for tests/test_review_bar.py:
- Normal interpreter: 52 passed.
- LC_ALL=C PYTHONUTF8=0: 51 passed, 1 skipped. The skip is narrowly conditional on os.fsencode refusing the non-ASCII filename; the normal run exercises that case.
- PYTHONOPTIMIZE=2: 52 passed, with pytest's expected warning about non-test assertions being disabled.
- Both
python -OO tools/check_dashes.py --helpandpython -OO tools/review_bar.py --helpexit successfully.
For a separate regression-sensitivity control, I compiled each tool with optimize=2 in memory, restoring only its original unguarded doc access. Both reproduced AttributeError; the guarded versions exited 0 for --help. No source edits or mutations were committed. The initial in-memory probe needed its temporary module registered for dataclass introspection; the reported results are from the corrected probe.
Ruff passes for tools/review_bar.py and tests/test_review_bar.py. Including tools/check_dashes.py additionally reports two E501 lines (56 and 70) already identical on the merge base; that broader lint scope is not claimed clean. Diff whitespace passes; checkout remains unchanged. Hosted exact-head CI, Conformance CI, Review bar, docs/package, Workflow lint, fuzz and CodeQL workflows are green.
This is not a fresh full-reference/conformance suite run or native Windows test, nor a review of every altered fixture generator. GitHub currently reports the PR unmergeable against main; please reconcile its integration conflict before merge, retaining the agreed #484 scope and running CI on the resulting head. The existing maintainer review and approval authority remain separate.
…python -OO (agentrust-io#484) - Add explicit encoding="utf-8" to bare Path.read_text() and Path.write_text() calls in tests/, conformance/tests/ (including test_manifest_ref_draft_vectors.py and manifest-ref-v0.3-draft/generate.py), and examples/verifier-token-conformance/gen_corpus.py, and decode git/pytest subprocess output as UTF-8 in tools/review_bar.py. - Guard module __doc__ access under python -OO (PYTHONOPTIMIZE=2) in tools/review_bar.py and tools/check_dashes.py. - Raise the referencing dependency floor from >=0.28 to >=0.28.4 in pyproject.toml and requirements/dev.in so it is satisfiable alongside jsonschema>=4.26.0. - Add a CI portability step in .github/workflows/ci.yml running LC_ALL=C PYTHONUTF8=0 pytest and PYTHONOPTIMIZE=2 pytest tests/test_review_bar.py. Closes agentrust-io#484 Signed-off-by: Raghuvar Durga <177662415+drk-ram@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…path (agentrust-io#484) Under LC_ALL=C PYTHONUTF8=0 on POSIX, sys.getfilesystemencoding() is 'ascii' with 'surrogateescape', so os.fsencode("test_été.py") raises UnicodeEncodeError before git is ever reached. Guard test_a_test_path_git_would_quote_is_still_found with os.fsencode() so it skips cleanly in the ASCII C-locale portability step while still running in the default UTF-8 test step. Signed-off-by: Raghuvar Durga <177662415+drk-ram@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
68b778c to
4f170fd
Compare
|
Rebased onto
|
noah-ing
left a comment
There was a problem hiding this comment.
Reviewed current head 4f170fd. The rebase addresses the earlier portability feedback: C-locale CI includes conformance, both tools run under -OO, and the review-bar help text retains its first paragraph. In an isolated Python 3.12 environment, C-locale focused root tests had 133 passes and two macOS worktree-cleanup failures that also reproduce on unchanged main; conformance had 781 passes and 5 xpasses. The -OO review-bar run passed 50 tests with those two cleanup tests excluded. Ruff, mypy, dash check, and diff whitespace check passed. Current-head hosted technical jobs pass. I found no blocking defect.
Nonblocking description correction: tests/test_acta_receipt_verifier.py merged on the base still has default-encoding Path calls, so the claim that zero bare calls remain throughout the repository is no longer literally true. Its current data is ASCII and the portability jobs pass. This Reviewer approval does not satisfy the named-maintainer gate.
rajnisht7
left a comment
There was a problem hiding this comment.
Reviewed the head 4f170fd, changes looks good overall, encoding fixes, -OO guards, dependency bump, and the CI portability job all check out. The non-ASCII filename skip under C locale is the right call.
One completeness note(can be followed up): tests/test_acta_receipt_verifier.py still has a few bare read_text() / write_text() calls. Current fixtures are ASCII so nothing breaks under LC_ALL=C, but adding encoding="utf-8" there would keep the whole suite consistent and avoid the same class of bug if a non-ASCII fixture shows up later.
dinakarjs
left a comment
There was a problem hiding this comment.
Re-reviewed exact rebased head 4f170fd91a1b029f200b38801e29177b74764034. The expanded C-locale conformance CI, both -OO tool invocations, first-paragraph help text and narrowly conditional non-ASCII filename skip address the earlier review comments. Current-head hosted CI test jobs pass. The named-maintainer approval workflow remains separate and failed when checked; I did not treat it as a technical test failure.
Fresh Linux/Python 3.12.14 environment with the head's hash-pinned reference and conformance dependencies: C-locale conformance has 781 passes and 5 xpasses. Review-bar tests have 52 passes normally and 52 under PYTHONOPTIMIZE=2 (with pytest's expected assertion warning); both -OO --help commands work. Ruff on src/tests/scripts, source mypy, dash and whitespace checks pass.
One additional local portability gap: the C-locale root run has 4,189 passes, 49 skips and one failure at test_the_javascript_side_agrees. Node 24.19.0's test process exits 0 and emits UTF-8 checkmarks, but subprocess text decoding uses ASCII and raises UnicodeDecodeError. The same subprocess call is unchanged on base e1560f3; this is an inherited environment-dependent test-output problem, not a new verifier regression. The output remains non-ASCII with Node-related overrides removed and with CI=true.
A DCO-signed two-file companion is ready for author adoption on this exact head:
dinakarjs@272e09b
It adds explicit UTF-8 to both Node subprocess reads and the seven remaining Acta Path reads/writes identified in the other reviews. No fixtures, verifier, dependencies or workflow change. With this companion, the full C-locale root suite passes: 4,190 passed, 49 skipped. The normal focused Acta/integer selection passes 266 tests; repository lint/type/style checks pass. Restoring only the locale-default Node-output decode makes its existing test fail again with UnicodeDecodeError; restoring the fix passes. For the Acta follow-up, an isolated copied expected.json with an unused non-ASCII note fails at import under the old default read and succeeds with explicit UTF-8; the unchanged ASCII control passes. No committed fixture was altered.
Published companion tree 39efa5a3c78b5573009270c3a456c82ad4f604a2 equals the tested tree. Please incorporate or explicitly defer the companion and refresh the PR description's zero-bare-calls claim, which is stale after the rebase. I am leaving a COMMENT rather than replacing current maintainer judgments; the earlier bounded tool review stands, and this supplies the new executed evidence and repair. No native Windows execution, full -OO-library safety or protocol conformance is claimed. Implemented/reviewed with OpenAI Codex assistance.
What this changes
Closes #484.
Contributor: Raghuvar Durga (@drk-ram)
Makes the test suite, conformance suite, fixture generators, and repository tooling portable across non-UTF-8 default encodings (
LC_ALL=C PYTHONUTF8=0/ Windowscp1252) andpython -OO(PYTHONOPTIMIZE=2), and raises thereferencingdependency floor so it is satisfiable alongsidejsonschema>=4.26.0, per@imran-siddique's scope ruling on #484:encoding="utf-8"to all barePath.read_text()andPath.write_text()calls acrosstests/(26 files),conformance/tests/(8 files, includingconformance/tests/test_manifest_ref_draft_vectors.pyandconformance/tests/vectors/manifest-ref-v0.3-draft/generate.pywithnewline="\n"), andexamples/verifier-token-conformance/gen_corpus.py.encoding="utf-8"tosubprocess.run(..., text=True)intools/review_bar.pysogit diff -zpath output andpytestoutput decode as UTF-8 underLC_ALL=C PYTHONUTF8=0.conformance/tests/test_measurement_harness.pysopython tools/check_dashes.pycontinues to pass once that file is edited past itsconformance/import-manifest.jsonsnapshot.__doc__underpython -OO(PYTHONOPTIMIZE=2):description=(__doc__ or "").split("\n", 1)[0]intools/review_bar.pyanddescription=(__doc__ or "").splitlines()[0] if __doc__ else ""intools/check_dashes.py.referencingfloor to>=0.28.4:referencing>=0.28toreferencing>=0.28.4inpyproject.tomlandrequirements/dev.into matchjsonschema>=4.26.0's requirement..github/workflows/ci.ymlrunningLC_ALL=C PYTHONUTF8=0 pytestandPYTHONOPTIMIZE=2 pytest tests/test_review_bar.py.Cold-reader review:
code-reviewsub-agent over the full diff against the Test suite and tools are not portable across default encodings andpython -OO;referencingfloor is unsatisfiable #484 scope and verified via AST inspection that zero bare.read_text()or.write_text()calls remain anywhere in the repository; the cold reader reported no issues.tests/test_review_bar.pyunderPYTHONUTF8=0during verification exposed thattools/review_bar.py'sgit()helper also usedsubprocess.run(..., text=True)withoutencoding="utf-8", causingtest_a_test_path_git_would_quote_is_still_foundto misdecode UTF-8 paths emitted bygit diff -z; addedencoding="utf-8"there as well.Type of change
Spec section
None (test suite, tooling, CI workflow, and package dependency metadata only).
Tests for a fix
Not a library or schema verification fix (
src/agentrust_trace/andschema/are unchanged), but the portability fixes are directly exercised by existing tests that fail onmainwhen the changes are switched off:PYTHONUTF8=0 pytest tests/test_regex_surface_parity.py tests/test_schema_regex_semantics.py tests/test_build_provenance_depth_doc.pyandPYTHONUTF8=0 pytest conformance/tests/test_manifest_ref_draft_vectors.pyfail withUnicodeDecodeError/ fixture mismatch without the explicitencoding="utf-8"reads and pass with them (conformance/tests: 778 passed, 5 xpassed; roottests/: 3,734 passed).PYTHONOPTIMIZE=2 pytest tests/test_review_bar.py -k "test_the_command_line_writes_the_summary_and_never_fails_the_build or test_an_error_inside_the_tool_still_writes_a_report_and_exits_zero"andpython -OO tools/check_dashes.pyfail withAttributeError: 'NoneType' object has no attribute 'split'/'splitlines'without the__doc__guards and pass with them.ruff check src tests scripts,python tools/check_dashes.py, andmypy src/agentrust_traceall pass.Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN: description -->in spec text