Audit test conventions and improve regression setup and assertions - #1082
Conversation
| {**read_csv_kwargs, "dtype": dict(read_csv_kwargs["dtype"])}, | ||
| ) | ||
| read_csv_kwargs = _pyarrow_read_csv_kwargs(types, **read_options) | ||
| expected = expected_frame(infer_string) |
There was a problem hiding this comment.
Self-review round one (implementation and test quality): CLEAN.
Reviewed the full seven-file diff: CSV option preparation and all 13 explicit frame expectations; dtype replacement versus overrides, duplicate names, missing values, datetime precision, and both string-inference contexts; filesystem-option sentinel identity; fresh Polars readers; synchronous/asynchronous disk-usage assertions and finally cleanup; audit evidence and convention/documentation integration. Shared fixture scopes and product code are unchanged. No actionable findings or deferred introduced regressions.
Validation of these contents: 108 offline pandas/Polars tests passed before and after; both real-AWS disk-usage cases passed; the four affected modules retained all 723 collected node IDs in the same order. Formatting, lint, and documentation checks passed. This is author self-review; the static review itself does not establish unrun suite coverage.
There was a problem hiding this comment.
Self-review round one repair follow-up: CLEAN.
Base: 2007620
Previous head: aef3ae1
Published repaired head: 6a25777
Verified both prior Git objects and inspected the bounded range-diff and all five repair files. Traced direct pandas and fsspec contracts: all 26 literal-frame checks remain, and 24 variants additionally compare explicit positional selections against pandas’ PyArrow engine. Duplicate and headerless names remain observable; selected dtype mappings are copied for the reference read, and intentional string-preservation exceptions remain literal. Both cleanup paths suppress only absent-prefix FileNotFoundError, with original write failures preserved in separate offline probes; other cleanup errors remain visible. Historical documentation and the Sphinx offline reference are consistent with the test setup.
Affected validation passed: 108 complete offline pandas/Polars tests, both real-AWS du cases (2 passed, no skips), 723 unchanged collection IDs in the same order, required format/lint, and documentation lint/current-source build. No actionable introduced defect or deferred repair. Author self-review; runtime evidence belongs to the author’s separate runs.
There was a problem hiding this comment.
Self-review round one documentation repair: CLEAN.
Base: 2007620
Previous head: 6a25777
Published head: 8c42428
Verified the prior objects and bounded range-diff. The only repair adds sibling Run tox and Record results headings in docs/testing.md; no test code, command, fixture, or public contract changes. The rendered HTML places general guidance outside SQLAlchemy suites. Required format/lint, Markdown lint, and current-source Sphinx build passed without new warnings. No actionable finding or deferred regression; author self-review.
There was a problem hiding this comment.
Self-review round one rebase follow-up: CLEAN.
Previous base/head: 2007620 / 8c42428
Current merge-base/head: 6888f4d / 1f08a42
Verified prior objects and reviewed the literal range-diff plus the upstream pandas production/test changes. The sole conflict was adjacent additions: the merged JSON CSV helper/regressions and this PR’s literal CSV expectation helpers. The resolution preserves every prior pandas AST node and all five upstream helper/test nodes exactly; the other six PR files are byte-identical to the prior reviewed head. Traced the new C-engine low_memory default through _read_csv and _JSONConverter, and checked local filesystem mocks, option lifetimes, and CSV/Pandas helper independence. The PR’s production-code diff against current master remains empty.
Validation: 132 complete offline pandas/Polars tests passed, including all 24 inherited JSON cases. Current master and the rebased PR collect the same 747 affected node IDs in the same order. Required format/lint and Markdown lint passed. Rebase doc rendering/current CI are still pending; no unrun result is claimed. No introduced regression or deferred conflict-resolution defect; author self-review.
There was a problem hiding this comment.
Self-review round one filesystem-base follow-up: CLEAN.
Previous base/head: 6888f4d / 1f08a42
Current merge-base/head: 0c19c8d / 60be248
The clean rebase retains all three patch-series entries unchanged. Inspected the new upstream filesystem diff and the direct pipe_file, async delegation, find/du/info and recursive-delete paths used by this PR. The 3- and 5-byte payloads use the unchanged single PutObject path; multipart upload/checksum changes do not reach them. Existing fixture lifetimes and the two test_du bodies are preserved exactly (AST comparison), and both offline modules are byte-identical. No production-code change belongs to this PR against its current base.
Required format/lint and Markdown lint passed. Both real-AWS du tests passed on this exact head (2 passed, no skips); current master and the PR collect the same 812 IDs in order. Current-source Sphinx rendering passed with existing API-doc warnings. The preceding local multiversion build completed through a temporary config-queue receive-before-join wrapper after its standard invocation stalled; normal CI documentation builds remain unchanged. No introduced interaction defect or deferred regression; author self-review.
There was a problem hiding this comment.
Self-review round one documentation relocation follow-up: CLEAN.
Merge-base: 0c19c8d
Previous published head: 60be248
Published repaired head: 03beabd
Verified both previous objects and inspected the literal git range-diff: the three previously reviewed entries are unchanged and one documentation-only commit removes docs/testing-audit.md plus its introduction and hidden toctree. Traced repository references and the rendered testing guide; no dangling audit reference remains, and Organize tests, offline instructions, Run tox, Record results, and GitHub Actions guidance remain available. AGENTS.md, all tests, product sources, dependencies and workflows are byte-identical to the previous reviewed head. Required just format, just lint, just docs lint, and current-source sphinx-build passed. This repair changes no runtime contract, so previous test results remain evidence for identical test contents; no new local AWS execution was needed. No actionable finding or deferred repair. Author self-review, not independent review.
There was a problem hiding this comment.
Self-review round one .env documentation follow-up: CLEAN.
Merge-base: 0c19c8d
Previous published head: 03beabd
Published repaired head: e72974b
Verified prior objects and inspected the literal bounded range-diff and final guide. Earlier four patch entries are unchanged; only docs/testing.md changes in the two new commits. The offline setup retains all four settings required by tests/init.py plus metadata exclusion, moves them to the existing gitignored .env pattern, and loads them through uv run --env-file .env. Session-hook and rerun-plugin exclusions, selected self-contained modules, lint-first order and the integration-test boundary remain intact. The first rendered revision exposed an unresolved heading reference; the final prose reference resolves that finding and introduces no testing-guide warning. Required format/lint and Markdown lint passed; final current-source rendering passed. The exact documented command passed 132 tests after clearing inherited AWS variables, proving the .env example supplies the required configuration. The subsequent repair changes only prose, so the validated .env contents and executable block are identical. No test/product/dependency/workflow changes or deferred defect. Author self-review.
|
|
||
| ## Validation boundaries | ||
|
|
||
| The initial audit is static source inspection. |
There was a problem hiding this comment.
Self-review round two (claims, compatibility, and operational evidence): CLEAN.
Checked the PR description and all new factual prose against the frozen source inventory, baseline references, actual validation logs, relevant pandas/Polars/fsspec callers, and CI path conditions. The 83-file manifest exactly matches the baseline; pinned source paths and ranges exist and the cited examples support their dispositions. Grouping exceptions retain upstream SQLAlchemy/PyHive conventions and attribution. The explicit time-only expectation still delegates its implicit current date to pandas, rather than claiming a fixed date. All 26 existing CSV variants and remaining stream/large-block checks remain selected.
Offline runs explicitly bypassed AWS session hooks; the two AWS cases ran after the prior local run ended. Product APIs, fixture ownership, and production AWS behavior are unchanged. The PR accurately separates static inspection, 108 local offline passes, 2 real-AWS passes, and pending CI. SQLAlchemy/Spark CI is intentionally excluded by the unchanged path filters for this diff; their fixtures and test sources were audited but not modified. No false guarantee, compatibility defect, or actionable operational finding identified. This is a separate author self-review, not independent review.
There was a problem hiding this comment.
Self-review round two repair follow-up: CLEAN.
Base: 2007620
Previous head: aef3ae1
Published repaired head: 6a25777
Audited the revised PR WHAT/WHY/TEST, historical report, offline guide, and changed test docstrings/comments as a separate claims pass. The documented uv command ran verbatim with 108 passes; the four placeholder values satisfy tests/init.py, --noconftest removes AWS hooks, and the rerun-plugin exclusion avoids its local socket. The explicit Sphinx reference resolves without new warnings. Dates, fixed baseline references, the 83-file inventory, preserved 723 IDs, and the literal-versus-library comparison boundary are accurate. The two AWS tests passed again after repair; missing-prefix probes cover both failure paths. Current fsspec _du/_find sources disprove the rejected directory-entry hypothesis. No production API, fixture ownership, or deployed AWS behavior changes. Current-head CI and independent follow-up are clearly still pending in the PR body, and static inspection is not claimed as complete-suite runtime proof. No false claim or actionable compatibility/operational defect remains.
There was a problem hiding this comment.
Self-review round two documentation repair: CLEAN.
Base: 2007620
Previous head: 6a25777
Published head: 8c42428
Separately checked the final PR claims, source and rendered heading hierarchy, and reporting instructions. General commit/version/command/failure/skip reporting is visibly a sibling subsection under Run tests. Commands and assertions remain identical to the previously validated revision; the PR explicitly records that the 108 offline and 2 AWS passes precede a documentation-only four-line heading change. The current doc build passed. Final independent confirmation and current-head Ready CI remain pending, accurately identified as such. No false claim or operational/compatibility defect; author self-review.
There was a problem hiding this comment.
Self-review round two rebase follow-up: CLEAN.
Previous base/head: 2007620 / 8c42428
Current merge-base/head: 6888f4d / 1f08a42
Separately audited current PR claims and the historical report after upstream integration. The report’s October 4 audit, original baseline pins and exclusion of JSON changes from that historical baseline remain accurate; its 83-file inventory is unchanged. Current validation is explicitly 132 passes and 747 preserved IDs; the extra 24 cases are inherited from merged #1080. The original 108 local passes, 2 real-AWS du passes and 2303-pass Ready CI are identified as earlier evidence. Filesystem source/tests and fixture ownership are unaffected by upstream pandas integration. The PR is Draft and accurately marks independent rebase confirmation and current-head CI as pending. Commands, supported dependency versions, parity exceptions and reporting requirements remain consistent. No false current-head claim or new caller/operational defect; author self-review.
There was a problem hiding this comment.
Self-review round two filesystem-base follow-up: CLEAN.
Previous base/head: 6888f4d / 1f08a42
Current merge-base/head: 0c19c8d / 60be248
Separately checked exact base/head claims, unchanged patch-series entries, current versus historical inventory/validation, and synchronous/asynchronous AWS effects. The 83-file audit still refers to its fixed original baseline. Current collection is 812 IDs, attributing 24 JSON and 65 filesystem additions to upstream. The 132 offline passes apply to byte-identical module contents; current filesystem integration is directly validated by two fresh real-AWS passes. S3 writes remain 3 and 5 bytes, cleanup remains scoped to each process schema/prefix, and no additional multipart traffic is introduced by these tests. The PR is Draft and prior 2303-pass Ready CI is correctly labeled earlier evidence; current-head CI and independent filesystem confirmation remain pending. Document rendering/lint and the local multiversion recovery are accurately separated from normal CI. No false current-head guarantee, caller regression or new operational defect; author self-review.
There was a problem hiding this comment.
Historical audit record preserved in this inline PR thread at the user's request.
The report is moved out of the repository documentation; its source-pinned evidence, priorities, intentional exceptions, and full 83-file inventory are retained below.
Only the Sphinx-specific offline reference is converted to a GitHub-renderable link.
Test conventions audit
This report records the source audit conducted on October 4, 2026, for issue #1079.
The baseline is commit 200762088e7b0bac45054f22e32a16aa0ce95dbb.
Source references below identify that baseline, so later edits do not change their meaning.
The findings and dispositions describe that audit and its implementation in PR #1082.
Reviewed areas
The inventory covers all 83 tracked files under tests/: 80 Python files, two Jinja query templates, and the SQLAlchemy profiles file.
The Python inventory includes 17 package initializers and all three conftest.py files.
Structural inspection covered class and function organization, fixture arguments and decorators, parameter construction, helper placement, assertions, and synchronous and asynchronous counterparts.
Focused source tracing checked the setup and expected-value patterns identified by that inspection.
The complete file inventory appears below.
| Area | Files | Conventions examined |
|---|---|---|
| Shared PyAthena tests and helpers | 15 | Cursor integration classes; standalone conversion helpers; object-oriented parser, model, result-set, and connection tests; session data and resource setup |
| Native asyncio tests and fixtures | 18 | Async cursor and dialect classes, async result-set helpers, per-test cursor contexts, connection teardown, and backend-specific cases |
| Arrow | 6 | Cursor classes, converter functions and classes, schema and precision assertions, mocked result-set setup |
| pandas | 7 | Cursor classes, function-oriented utility tests, reader lifetimes, CSV options and expected frames, dtype and option-context coverage |
| Polars | 5 | Cursor and converter tests, typed local files, failure after partial reads, and one-shot iterator inputs |
| S3FS cursors and readers | 4 | Cursor fixtures, custom converters, CSV NULL versus empty-string assertions, and reader context managers |
| Spark | 4 | Session and calculation fixtures, event-based concurrency helpers, cancellation assertions, and Future-based async behavior |
| Filesystem | 9 | Class fixtures, stubbed provider calls, path/value parametrization, sync/async operations, cleanup, and tests without observable assertions |
| PyAthena SQLAlchemy tests | 8 | Compiler and type unit tests, engine integration classes, local fixture reuse, SQL expression parameters, and reflected metadata assertions |
| SQLAlchemy compliance suite | 4 | Upstream inheritance, combinations and requirements, plugin hooks, sync/async selection, and explicit skips |
| Shared environment initializer | 1 | Required environment values and per-process schema identity |
| Query templates | 2 | Database setup/teardown, schema interpolation, and license notices |
The compliance suite imports additional tests from the installed SQLAlchemy package.
Those upstream implementations and generated cases are outside the project-owned source inventory.
The audit does not establish complete runtime coverage or the correctness of every assertion.
The audit excluded changes not merged into this baseline, including the pandas JSON changes associated with issue #1078.
Findings addressed
The priorities describe coverage and maintenance impact, rather than product-defect severity.
| Priority | Baseline evidence | Impact | Disposition |
|---|---|---|---|
| High | tests/pyathena/filesystem/test_s3.py:4432 and tests/pyathena/filesystem/test_s3_async.py:1491 | Both test_du bodies contain only pass. A broken disk-usage implementation still produces two passing tests. |
Replace both placeholders with real S3 file-size, total, depth, and single-file assertions. Preserve the names, use the existing filesystem fixtures, and remove the test objects in finally. |
| Medium | tests/pyathena/pandas/test_result_set.py:265 and tests/pyathena/pandas/test_result_set.py:305 | Parameter evaluation repeatedly patches result-set properties and derives CSV options, including repeated default dtype construction. An option-building error prevents collection of unrelated tests in the module. | Store input metadata and options in the parameters. Build the options once per invocation, inside the pandas option context. Remove the unused base-initializer patch; __new__ already bypasses __init__. |
| Medium | tests/pyathena/pandas/test_result_set.py:391 | The expectation reads the CSV with two engines and conditionally replaces columns and missing values. The reader has to reconstruct the intended contract from this algorithm. | Define typed expected frames from explicit values for the 13 existing cases. Retain direct pandas PyArrow-engine comparisons for explicitly selected column positions where the contracts agree. Keep both future.infer_string settings, check_exact=True, column/index checks, and the difference between replacing a dtype mapping and overriding individual entries. |
| Medium | tests/pyathena/polars/test_result_set.py:259 | A generator is constructed in the parameter definition. Repeating the case can reuse a closed reader, making the post-close empty result a vacuous assertion. | Parameterize the reader kind and construct a fresh DataFrame or generator during each test invocation. Keep both case IDs. |
| Low | tests/pyathena/pandas/test_result_set.py:79 | Module-level mocks are used only as filesystem identities while read_parquet is patched. They introduce mutable mock state without asserting any mock behavior. |
Use distinct sentinels while preserving the identity and option-precedence assertions. |
| Low | tests/pyathena/pandas/test_result_set.py:439 | The expected CSV parse contributes only a column-name comparison whose expected name is already literal. | Assert the explicit column name and preserved 007 value directly. |
| Medium | AGENTS.md:84, tests/pyathena/test_parser.py:18, and tests/pyathena/pandas/test_util.py:62 | The grouping guidance does not explain established object-oriented unit classes or function-oriented utility tests that use AWS fixtures. Applying it mechanically changes test selection without improving the assertions. | Clarify the default forms and their purposeful exceptions in AGENTS.md and the testing guide. Retain the existing grouping and fixture scopes. |
The all-types expectation shares a typed literal frame across cases.
Its time-only field still uses pandas' datetime conversion to supply today's date, as the existing input requires.
The literal frame does not parse a reference CSV or reproduce PyAthena's CSV conversion loop.
Separate library comparisons cover numeric, temporal, unmapped, and explicitly categorized columns, including duplicate and headerless column names.
Mapped string columns retain literal assertions for PyAthena's intentional preservation of string values and missing values.
Intentional differences retained
| Evidence | Rationale and disposition |
|---|---|
| tests/pyathena/test_converter.py:19 and tests/pyathena/test_parser.py:18 | Standalone conversion functions and classes grouping a parser object's behavior are both useful. Preserve both forms. |
| tests/pyathena/pandas/test_util.py:451 | Utility functions that write DataFrames use real cursor fixtures. Their function-oriented grouping does not make them offline tests or require a class conversion. |
| tests/pyathena/pandas/test_result_set.py:52 | Comparing joined chunks with a whole-file pandas read directly expresses the iterator's compatibility contract. Keep this library oracle. |
| tests/pyathena/pandas/test_result_set.py:139 | The DDL test combines C-engine comparison with literal numeric-looking names, engine selection, and stream-closure assertions. These observable assertions keep the comparison tied to its specific contract. |
| tests/pyathena/pandas/test_cursor.py:1945 and tests/pyathena/s3fs/test_cursor.py:543 | These parameters construct separate, configured converter values for explicit default/managed cases. Unlike a one-shot reader, they are not consumed. Keep the fixture inputs and managed-storage skip conditions; a future change that mutates the supplied converter should give it a per-invocation lifetime. |
| tests/pyathena/sqlalchemy/test_compiler.py:621 | SQLAlchemy type and expression construction is part of the compiler input. Keep these declarative parameters and their dialect variants. Primitive parameter values also have useful generated IDs; custom IDs are needed when the generated names obscure the scenario. |
| tests/sqlalchemy/test_suite.py:35 and tests/sqlalchemy/conftest.py:11 | Compliance tests inherit upstream classes and use SQLAlchemy's combinations, requirements, and pytest plugin. Preserve these interfaces and the sync/async database selection. |
| tests/sqlalchemy/test_suite.py:974 | Skipped overrides carry concrete Athena limitations. They differ from the unmarked filesystem placeholders because they report missing coverage as skipped. Preserve the reasons and test selection. |
| NOTICE:29 | The notice identifies surviving PyHive-derived cursor/dialect tests, including a native-async port. Preserve attribution and framework behavior when reorganizing adapted material. |
| tests/pyathena/conftest.py:225 and tests/pyathena/aio/conftest.py:21 | Sync and native-async fixtures own their connections and cursor contexts with their corresponding teardown forms. The implementations need different syntax; a shared replacement is not required for consistency. |
| tests/pyathena/arrow/test_cursor.py:38 and tests/pyathena/aio/arrow/test_cursor.py:22 | Both execution styles assert binary NULL versus empty bytes. Backend-specific cases and Future versus native-async result handling justify differences in the complete method inventories. Preserve meaningful corresponding scenarios rather than forcing identical test lists. |
The session hooks in tests/pyathena/conftest.py:15 prepare real AWS resources even for a selected pure-logic test.
This existing behavior is documented in the testing guide.
The organizational changes preserve it; an offline invocation must explicitly exclude those hooks and select self-contained modules.
The offline test instructions include the required placeholder environment values and a validated command.
Validation boundaries
The initial audit is static source inspection.
Runtime results belong to the implementation revision and are recorded separately in PR #1082's TEST section, with commands, dependency versions, and skipped coverage.
The validation scope comprises comparison of the affected modules' collected node IDs, the self-contained pandas and Polars tests offline, and both disk-usage cases against the configured AWS environment.
The applicable PyAthena suite runs in CI when the implementation PR is Ready.
The fixes above address the identified actionable inconsistencies without a broad grouping conversion.
Implementation review also retained the pandas compatibility comparisons, documented the offline invocation, and protected the disk-usage tests' original write errors when cleanup encounters an absent prefix.
File inventory
tests/__init__.py
tests/pyathena/__init__.py
tests/pyathena/aio/__init__.py
tests/pyathena/aio/arrow/__init__.py
tests/pyathena/aio/arrow/test_cursor.py
tests/pyathena/aio/conftest.py
tests/pyathena/aio/pandas/__init__.py
tests/pyathena/aio/pandas/test_cursor.py
tests/pyathena/aio/polars/__init__.py
tests/pyathena/aio/polars/test_cursor.py
tests/pyathena/aio/s3fs/__init__.py
tests/pyathena/aio/s3fs/test_cursor.py
tests/pyathena/aio/spark/__init__.py
tests/pyathena/aio/spark/test_cursor.py
tests/pyathena/aio/sqlalchemy/__init__.py
tests/pyathena/aio/sqlalchemy/test_base.py
tests/pyathena/aio/test_common.py
tests/pyathena/aio/test_connection.py
tests/pyathena/aio/test_cursor.py
tests/pyathena/aio/test_result_set.py
tests/pyathena/arrow/__init__.py
tests/pyathena/arrow/test_async_cursor.py
tests/pyathena/arrow/test_converter.py
tests/pyathena/arrow/test_cursor.py
tests/pyathena/arrow/test_result_set.py
tests/pyathena/arrow/test_util.py
tests/pyathena/conftest.py
tests/pyathena/filesystem/__init__.py
tests/pyathena/filesystem/test_init.py
tests/pyathena/filesystem/test_s3.py
tests/pyathena/filesystem/test_s3_async.py
tests/pyathena/filesystem/test_s3_core.py
tests/pyathena/filesystem/test_s3_errors.py
tests/pyathena/filesystem/test_s3_executor.py
tests/pyathena/filesystem/test_s3_object.py
tests/pyathena/filesystem/test_s3_path.py
tests/pyathena/pandas/__init__.py
tests/pyathena/pandas/test_async_cursor.py
tests/pyathena/pandas/test_converter.py
tests/pyathena/pandas/test_cursor.py
tests/pyathena/pandas/test_reader.py
tests/pyathena/pandas/test_result_set.py
tests/pyathena/pandas/test_util.py
tests/pyathena/polars/__init__.py
tests/pyathena/polars/test_async_cursor.py
tests/pyathena/polars/test_converter.py
tests/pyathena/polars/test_cursor.py
tests/pyathena/polars/test_result_set.py
tests/pyathena/s3fs/__init__.py
tests/pyathena/s3fs/test_async_cursor.py
tests/pyathena/s3fs/test_cursor.py
tests/pyathena/s3fs/test_reader.py
tests/pyathena/spark/__init__.py
tests/pyathena/spark/test_async_cursor.py
tests/pyathena/spark/test_common.py
tests/pyathena/spark/test_spark_cursor.py
tests/pyathena/sqlalchemy/__init__.py
tests/pyathena/sqlalchemy/test_array.py
tests/pyathena/sqlalchemy/test_base.py
tests/pyathena/sqlalchemy/test_compiler.py
tests/pyathena/sqlalchemy/test_map.py
tests/pyathena/sqlalchemy/test_struct.py
tests/pyathena/sqlalchemy/test_temporal.py
tests/pyathena/sqlalchemy/test_types.py
tests/pyathena/tables.py
tests/pyathena/test_async_cursor.py
tests/pyathena/test_connection.py
tests/pyathena/test_converter.py
tests/pyathena/test_cursor.py
tests/pyathena/test_formatter.py
tests/pyathena/test_glue.py
tests/pyathena/test_model.py
tests/pyathena/test_options.py
tests/pyathena/test_parser.py
tests/pyathena/test_result_set.py
tests/pyathena/test_util.py
tests/pyathena/util.py
tests/resources/queries/create_database.sql.jinja2
tests/resources/queries/drop_database.sql.jinja2
tests/sqlalchemy/__init__.py
tests/sqlalchemy/conftest.py
tests/sqlalchemy/profiles.txt
tests/sqlalchemy/test_suite.py
There was a problem hiding this comment.
Self-review round two documentation relocation follow-up: CLEAN.
Merge-base: 0c19c8d
Previous published head: 60be248
Published repaired head: 03beabd
Separately audited the relocation claims and PR WHAT/WHY/TEST against the GitHub-returned inline audit body, original report, remaining guide, rendered section anchors and workflow filters. The full 83-file historical report is preserved at #1082 (comment); only its Sphinx-specific offline reference was converted to a pinned GitHub link whose target and heading exist. Priorities, source evidence, exceptions, inventory and original baseline pins are unchanged. Permanent conventions remain in AGENTS.md and docs/testing.md. The PR correctly identifies previous runtime and CI results separately from the pending documentation repair and remains Draft. The new local lint/render checks passed; existing API-documentation warnings are separate from the testing guide. The whole PR still changes tests, so its Ready event requires current-head Test CI; independent follow-up and that CI remain pending. No false current-head validation claim or compatibility/operational defect. Author self-review.
There was a problem hiding this comment.
Self-review round two .env documentation follow-up: CLEAN.
Merge-base: 0c19c8d
Previous published head: 03beabd
Published repaired head: e72974b
Separately checked the AWS versus offline-only setup claims, existing guide commands, .env ignore rule, exported source initializer, rendered sections and actual validation. A configured AWS .env may be reused; contributors without AWS resources can create the clearly scoped offline-only example. Its bucket/workgroups are placeholders for the initializer and are not described as usable integration resources. Metadata exclusion is attributed specifically to that example, preserving the normal AWS credential-chain guidance for existing setups. The exact .env-based offline command passed 132 tests with inherited AWS variables cleared; lint ran first. The worktree .env contains only documented placeholders, is gitignored and was created only after proving no prior file/symlink existed; the main checkout configuration was untouched. The final prose-only repair removes the Sphinx warning without changing the tested command. The 2492-pass Ready Test result belongs to previous head 03beabd, and new independent confirmation/current-head CI remain pending while Draft. No false current-head proof or operational/compatibility defect. Author self-review.
| await fs._pipe_file(first, b"abc") | ||
| await fs._pipe_file(second, b"12345") | ||
| assert await fs._du(directory) == 8 | ||
| assert await fs._du(directory, total=False) == { |
There was a problem hiding this comment.
Relayed independent static review by Claude Code, model claude-opus-5-5, profile max, effort high. Verified first-party Claude.ai Max authentication; no Enterprise/API/provider override. Session: 113e6092-a1e7-47d0-96ad-f51235d7fe7f.
Base: 2007620
Head: aef3ae1
Review result: FINDINGS (four items). High hypothesis: inherited AsyncFileSystem._du might include directory entries, making this two-file mapping fail. The reviewer explicitly lacked the installed dependency source and based this item on recalled fsspec behavior.
Author verification: rejected. In the locked fsspec 2026.9.0, asyn.py:1002-1011 calls _find directly without withdirs=True; _find defaults to withdirs=False. The actual source does not use the hypothesized _expand_path here. Both synchronous and asynchronous cases passed against real AWS (2 passed, no skips). The dependency sources will be supplied for the bounded follow-up.
The reviewer inspected all seven changed files, relevant pandas/Polars/filesystem contracts and fixtures, source references, environment/configuration, and provenance. Review tools were restricted to Read/Glob/Grep on the exported tracked snapshots and diff; no edits, commands, tests, GitHub, memory, or web access. All original package-file fingerprints remained unchanged. The author’s separate option comparison generated one temporary pyc file in the baseline export; it did not change a reviewed source. Runtime validation above is author evidence, not reviewer execution.
There was a problem hiding this comment.
The completed independent follow-up confirms rejection against the provided locked dependency source: fsspec asyn.py:1002-1011 calls _find without withdirs=True, and AioS3FileSystem._find defaults withdirs=False and delegates that value. The two-file mapping is correct. Review session: 984dcf63-5640-4ee6-a8e6-51028003e814 (claude-opus-5-5, Max, high). Both real-AWS cases also passed again after the separate cleanup repair. No production behavior was changed for this hypothesis.
| {**read_csv_kwargs, "dtype": dict(read_csv_kwargs["dtype"])}, | ||
| ) | ||
| read_csv_kwargs = _pyarrow_read_csv_kwargs(types, **read_options) | ||
| expected = expected_frame(infer_string) |
There was a problem hiding this comment.
Relayed independent static review by Claude Code, model claude-opus-5-5, profile max, effort high. Verified first-party Claude.ai Max authentication; no Enterprise/API/provider override. Session: 113e6092-a1e7-47d0-96ad-f51235d7fe7f.
Base: 2007620
Head: aef3ae1
Medium finding: replacing the algorithmic reference frame with literals also removed direct pandas PyArrow-reader parity checks. A future pandas release could change numeric/temporal dtypes or duplicate/headerless names while all literal assertions remain green, despite the reader’s compatibility contract.
Author disposition: accepted. Retain all explicit expected frames and add a direct library comparison on explicitly selected column positions where the contracts agree. Keep intentional mapped-string preservation exceptions literal. The repaired contents will receive validation, both self-review perspectives, and independent follow-up.
There was a problem hiding this comment.
Repaired in 6a25777. All 26 explicit frame cases remain. Twenty-four variants additionally compare explicit positional selections against pd.read_csv(engine='pyarrow'); the all-string exception retains its literal contract. Numeric/temporal/null, categorized and unmapped columns, duplicate names, and headerless names remain observable without reproducing the CSV conversion loop. The complete pandas/Polars modules pass (108 tests), with all 723 affected collection IDs unchanged. Both self-review repair passes are recorded above; independent follow-up is in progress.
|
|
||
| ## Validation boundaries | ||
|
|
||
| The initial audit is static source inspection. |
There was a problem hiding this comment.
Relayed independent static review by Claude Code, model claude-opus-5-5, profile max, effort high. Verified first-party Claude.ai Max authentication; no Enterprise/API/provider override. Session: 113e6092-a1e7-47d0-96ad-f51235d7fe7f.
Base: 2007620
Head: aef3ae1
Low finding: the published audit uses transient PR/worktree wording, and its offline guidance omits the four configuration values still required by tests/init.py and a copyable hook-excluding invocation.
Author disposition: accepted. Date and pin the historical report, link the implementation record explicitly, and document the validated self-contained invocation with all four placeholder values, --noconftest, and the rerun-plugin exclusion.
There was a problem hiding this comment.
Repaired in 6a25777. The report is dated October 4, 2026, and fixed to its historical baseline and implementation record. The testing guide now supplies a copyable offline uv command, all four required placeholder values, EC2-metadata exclusion, --noconftest, and disabled rerun-plugin setup. That exact command passed 108 tests. Documentation lint and the current-source Sphinx build passed; the explicit offline cross-reference resolves without new warnings. Independent follow-up is in progress.
| } | ||
| assert fs.du(directory, maxdepth=1) == 3 | ||
| assert fs.du(first) == 3 | ||
| finally: |
There was a problem hiding this comment.
Relayed independent static review by Claude Code, model claude-opus-5-5, profile max, effort high. Verified first-party Claude.ai Max authentication; no Enterprise/API/provider override. Session: 113e6092-a1e7-47d0-96ad-f51235d7fe7f.
Base: 2007620
Head: aef3ae1
Low finding (also tests/pyathena/filesystem/test_s3_async.py:1507): if the first write fails before the prefix exists, cleanup can raise FileNotFoundError and become the reported failure, obscuring the original write error.
Author disposition: accepted. Suppress only FileNotFoundError during finally cleanup in both execution styles; continue surfacing other cleanup failures. Validate normal AWS assertions and the missing-prefix failure path.
There was a problem hiding this comment.
Repaired in 6a25777 in both sync and async test_du. Finally cleanup suppresses only FileNotFoundError. Separate offline probes invoke the actual test methods with a first-write failure and absent-prefix cleanup; both preserve the original exception. Both real-AWS cases passed again after repair (2 passed, no skips), so normal cleanup and assertions are also validated. Independent follow-up is in progress.
| `--noconftest` excludes the AWS session hooks and fixtures; disabling the rerun plugin also avoids its local socket setup in restricted environments. | ||
| Use this invocation only for self-contained modules; integration tests need their normal fixtures and a real AWS environment. | ||
|
|
||
| ### SQLAlchemy suites |
There was a problem hiding this comment.
Relayed bounded independent follow-up: FINDINGS, one new Low documentation-structure issue.
Reviewer: Claude Code, claude-opus-5-5, max profile, effort high; first-party Max authentication verified, no Enterprise/API/provider override. Actual result modelUsage contains only claude-opus-5-5 (firstParty). Session: 984dcf63-5640-4ee6-a8e6-51028003e814.
Base: 2007620
Previous head: aef3ae1
Reviewed head: 6a25777
All four original dispositions verified: the async directory-entry hypothesis is rejected against actual fsspec 2026.9.0 _du/_find sources; pandas parity coverage, historical/offline instructions, and cleanup diagnostics are resolved. Explicit pandas positional selections reproduce the baseline’s direct comparisons, including category, date override, duplicate and headerless names. Headerless mapped-string values remain literal-only comparisons, a narrow coverage limit the reviewer identified as non-failing. The runtime word 'validated' cannot be established by this static review; the author separately ran the documented command successfully.
New Low finding at docs/testing.md:155-174: adding the SQLAlchemy suites subsection nests general tox and result-reporting guidance beneath it. A documentation reader could interpret required version/command/skip reporting as specific to SQLAlchemy and omit it for other changes. Author disposition: accepted; separate Run tox and Record results headings restore their general scope.
Covered every repair hunk, direct pandas/fsspec contracts and dependency sources, env/fixture/plugin configuration, and documentation structure. Static review only: no test/lint/build/command execution, GitHub/web/MCP/memory access, or edits. All 857 review-package file fingerprints and the PR worktree remained unchanged during review. PyArrow inference source and pytest/plugin implementation were not included, so those semantics were not independently traced.
There was a problem hiding this comment.
Resolved in 8c42428 by adding sibling Run tox and Record results headings. Required format/lint, Markdown lint, and actual current-source Sphinx build passed; the rendered HTML confirms the general hierarchy. Both author self-review perspectives passed. The final bounded independent confirmation is CLEAN (claude-opus-5-5, Max, high, session 1cb5a6b7-0340-427b-8a2a-ce7e8029b938).
| uv run --env-file .env just tox | ||
| ``` | ||
|
|
||
| ### Record results |
There was a problem hiding this comment.
Relayed final bounded independent confirmation: CLEAN. The prior Low documentation-heading finding is resolved, with no new actionable issue.
Reviewer: Claude Code, claude-opus-5-5, max profile, effort high; first-party Max authentication verified, no Enterprise/API/provider override. Actual result modelUsage contains only claude-opus-5-5. Session: 1cb5a6b7-0340-427b-8a2a-ce7e8029b938.
Base: 2007620
Previous head: 6a25777
Published reviewed head: 8c42428
Reviewed the literal four-line repair and range-diff, the full current testing guide and corresponding previous region, and directly affected audit/contribution/AGENTS requirements, MyST configuration, Markdown heading rules, and cross-reference uses. Run tox and Record results are now H3 siblings of SQLAlchemy suites under Run tests. Body text and the offline label remain unchanged; no existing link depends on the new headings. Earlier patch-series entries are unchanged, so the three resolved original findings and the rejected async hypothesis were not reopened.
Static review only: no commands, tests, lint/build, GitHub/web/MCP, memory, edits, or external context. Rendering and lint conclusions were inferred from source; the author separately validated actual HTML and lint. All 567 package-file fingerprints and the PR worktree remained unchanged during review. Combined with the initial full review and completed repair follow-up, no required independent finding remains unresolved.
8c42428 to
1f08a42
Compare
| read_csv_kwargs = _pyarrow_read_csv_kwargs(types, **read_options) | ||
| expected = expected_frame(infer_string) | ||
| actual = _read_csv_with_pyarrow(io.BytesIO(data.encode()), read_csv_kwargs) | ||
| assert_frame_equal(actual, expected, check_exact=True) |
There was a problem hiding this comment.
Relayed bounded independent rebase follow-up: CLEAN. No introduced conflict-resolution or upstream-interaction defect; no separate existing actionable issue found.
Reviewer: Claude Code, claude-opus-5-5, max profile, effort high; first-party Max authentication verified, no Enterprise/API/provider override. Actual result modelUsage contains only claude-opus-5-5. Session: 2b3f6abe-09c2-4247-9dec-d78445e5bb6a.
Previous base/head: 2007620 / 8c42428
Current merge-base/published head: 6888f4d / 1f08a42
Reviewed the literal range-diff and upstream diff, overlapping test helpers/decorators/assertions in all three snapshots, the upstream C-engine low_memory/_JSONConverter path and its direct callers/options, and affected historical/offline documentation. Upstream JSON helper/tests survive verbatim, as do the PR’s explicit-frame/parity blocks, IDs, string-inference contexts, copied reference dtype and sentinels. The first commit’s range-diff change only realigns context; the other two patch entries are unchanged. The new low_memory default is confined to _read_csv with C-engine JSON converters; the parity test calls the PyArrow readers directly without JSON, and the existing DDL comparison uses only strings. No option or filesystem-mock lifetime leaks between groups.
Historical baseline pins/exclusion wording remain accurate. The reviewer noted that runtime ID comparisons must use current merge-base or attribute the 24 upstream additions; author records do so (747 IDs and 132 offline passes).
Static review only: no tests, collection, lint/build, commands, edits, GitHub/web/MCP/memory, or external context. Counts and preservation were inspected manually by the reviewer, not mechanically executed. All 858 package-file fingerprints and the PR worktree remained unchanged during review. Author AST/collection/runtime checks are separate evidence.
1f08a42 to
60be248
Compare
| try: | ||
| fs.pipe_file(first, b"abc") | ||
| fs.pipe_file(second, b"12345") | ||
| assert fs.du(directory) == 8 |
There was a problem hiding this comment.
Relayed bounded independent filesystem-base confirmation: CLEAN.
Reviewer: Claude Code, claude-opus-5-5, max profile, effort high; first-party Max authentication verified, no Enterprise/API/provider override. Actual result modelUsage contains only claude-opus-5-5. Session: bd4c4c07-a6d9-40e6-82fc-cd5ad84f0677.
Previous base/head: 6888f4d / 1f08a42
Current merge-base/published head: 0c19c8d / 60be248
All three patch-series entries remain unchanged. The reviewer checked the test_du hunks, newly merged filesystem diff, direct write/find/info/delete paths, imports and class fixtures, and the boundary to multipart/checksum/cleanup changes. The 3-/5-byte writes use the unchanged single PutObject path, including native-async delegation. Listing, disk usage, path normalization, recursive deletion and missing-prefix cleanup do not call the changed multipart primitives. New upstream tests use distinct UUID prefixes; mock/monkeypatch scopes restore shared state, and permanent assignments target uncached throwaway filesystems. No fixture leak or integration regression found. Historical audit pins remain accurate; pandas/Polars sources are unaffected.
Static review only: no tests, S3 access, collection, lint/build, commands, edits, GitHub/web/MCP/memory or external context. Unchanged fsspec total/maxdepth semantics were not reopened, and default block size was read rather than measured. All 858 package-file fingerprints and the PR worktree remained unchanged during review. The author separately reran both du cases on real AWS (2 passed) and preserved all 812 collected IDs.
| Use literal expected values for contract-specific behavior. | ||
| A library comparison is useful when matching that library is the contract, such as joining pandas chunks versus reading the whole file. | ||
| Express intentional differences directly rather than recreating the implementation in the expected-value builder. | ||
| Retain dtype and schema checks, option contexts, resource cleanup, and equivalent synchronous and asynchronous scenarios where applicable. |
There was a problem hiding this comment.
Relayed bounded independent documentation-relocation confirmation: CLEAN.
No actionable defect found within this follow-up.
Reviewer: Claude Code, claude-opus-5-5, max profile, effort high; first-party Max authentication verified, no Enterprise/API/provider override.
Actual result modelUsage contains only claude-opus-5-5 (firstParty).
Session: 9cde76a8-2c03-404d-b3a7-a49451a8bf09
Merge-base: 0c19c8d
Previous published head: 60be248
Published reviewed head: 03beabd
Reviewed the literal git range-diff, complete repair diff, remaining testing guidance and directly affected documentation/configuration references.
The three previous patch-series entries are unchanged; the new commit only removes the historical report and its introduction/hidden toctree.
No reference to the deleted page remains. Permanent conventions, the offline command, and heading/fence structure are intact.
The retained audit artifact matches the deleted report apart from its Sphinx-only offline reference, now a normal Markdown link to the previous head's existing unique heading.
Its 83-file inventory, evidence, priorities, exceptions, validation boundaries and 27 baseline-pinned references are preserved.
The unused testing-offline label remains a valid stable target and is not a defect.
Static review only: Read/Glob/Grep in exported tracked sources, no commands, edits, tests, collection, lint/build, GitHub/web/MCP/memory or subagents.
The reviewer did not establish actual posting or runtime link/build success. The author separately verified the exact GitHub-returned report body and ran required lint plus current-source rendering; standard documentation-build CI also passed.
All 863 review-package file fingerprints and the PR worktree remained unchanged during review.
Combined with the completed initial review and preceding confirmations, no required independent finding remains unresolved.
| After `just lint`, load `.env` and run: | ||
|
|
||
| ```bash | ||
| uv run --env-file .env pytest --noconftest -p no:rerunfailures -q \ |
There was a problem hiding this comment.
Relayed bounded independent .env documentation confirmation: CLEAN.
No actionable defect found in this follow-up.
Reviewer: Claude Code, claude-opus-5-5, max profile, effort high; verified first-party Max authentication, no Enterprise/API/provider override.
Actual result modelUsage contains only claude-opus-5-5 (firstParty).
Session: 81e3300a-4349-4024-a0fb-816b28d21fd1
Merge-base: 0c19c8d
Previous published head: 03beabd
Published reviewed head: e72974b
The reviewer inspected all final repair hunks and the literal range-diff; the four preceding entries remain unchanged.
Both real and offline .env examples supply the four import-time values required by tests/init.py; the placeholder staging URL parses correctly.
The metadata-disabling claim is properly limited to the offline-only example.
The documented modules have no static client/session/filesystem creation path, so loading an existing AWS configuration does not introduce an identified AWS call.
uv run --env-file .env agrees with the existing guide and worktree-env script; .env is ignored, conftest/rerun-plugin exclusions are retained, and no pytest addopts/dotenv plugin alters the flow.
The final prose reference avoids an unresolved MyST heading target while preserving the offline label and valid code fences.
Non-defect boundary: offline placeholders persist in .env and are unsuitable for an AWS suite; the existing real-configuration instructions and explicit offline-only/integration boundary already explain the distinction.
No additional finding was raised.
Static source review only: Read/Glob/Grep in exported tracked sources; no commands, edits, tests, collection, lint/build, AWS, GitHub/web/MCP/memory access or subagents.
The reviewer did not independently execute uv or measure botocore behavior.
All 861 package-file fingerprints and the PR worktree remained unchanged.
The author separately ran the exact .env invocation with inherited AWS variables cleared (132 passed), required lint, and final current-source rendering without testing-page warnings; standard documentation-build CI also passed.
No independent finding remains unresolved.
WHAT
Audit all 83 tracked files under
tests/at the original200762088e7b0bac45054f22e32a16aa0ce95dbbbaseline.Preserve the complete historical evidence, priorities, exceptions, and inventory in the inline audit record.
Keep permanent conventions in
AGENTS.mdanddocs/testing.md; the one-off audit document and its documentation links are absent from the final PR diff.Document offline configuration in a gitignored
.env, reuse an existing AWS configuration when available, and load it consistently withuv run --env-file .env.Build pandas CSV options during execution and use typed literal expected frames for all 13 cases under both string-inference settings.
Retain direct pandas PyArrow-reader comparisons on explicit columns where the contracts agree, and literal assertions for intentional string-preservation differences.
Replace identity-only mocks with sentinels and construct a fresh Polars reader per invocation.
Replace both synchronous and asynchronous disk-usage placeholders with real file-size, total, depth, and single-file assertions; cleanup preserves an original write failure when the prefix is absent.
WHY
Closes #1079.
The audit identified collection-time setup, complex expected-value construction, a reusable one-shot reader, and two tests containing only
pass.The conventions explain purposeful class/function grouping and preserve fixture scopes, case IDs, upstream SQLAlchemy interfaces, and adapted-code attribution.
Product code and public APIs are unchanged by this PR.
TEST
Current published head:
e72974b4823aa83873784dbc3345bb1168ac449b.Merge-base:
0c19c8daf8b6f8d7ded579b459d1595fff8fe1d9.The documentation follow-ups change only the two documentation paths; all tests, product sources, dependencies, workflows, and
AGENTS.mdare identical to previously reviewed head60be248a0e4fced0e8fbe2d30dfe00c228582689.just format,just lint, andjust docs lintpassed.uv run sphinx-build -b html -E docs <temporary-output-directory>passed. Existing API documentation emits warnings; none names the testing guide. The permanent sections remain, and no audit link remains in the rendered guide..envinvocation passed 132 tests with inherited AWS variables cleared. The executable example and .env contents are identical on the final prose-repaired head.Previous runtime validation applies to unchanged test contents:
For offline execution, create
.envwith the placeholder values in the testing guide, then run:The real-AWS pair uses an AWS-configured
.envand its real test resources:Two distinct self-review rounds, earlier independent reviews, and the documentation repair's two bounded self-review passes are recorded inline.
The independent documentation follow-up uses Claude Code
claude-opus-5-5, Max profile, effort high, with verified first-party Max authentication, restricted read-only tools, and frozen exported sources.The prior audit-relocation follow-up is CLEAN on
03beabd2b4c6feb6686bbced597ccfb097d6cd68(session9cde76a8-2c03-404d-b3a7-a49451a8bf09).The current .env follow-up is CLEAN on the published head above (session
81e3300a-4349-4024-a0fb-816b28d21fd1); static review is separate from runtime validation.The current-head Ready Test workflow passed on Python 3.14.7: 2492 passed, 1 skipped, 13 warnings, with no failures.
The suite retains the existing explicitly skipped S3FS
test_executemany, which requires theinsert_testtable in the test environment.License, Python lint, Markdown lint, and standard documentation-build CI passed on the current head.
The PR is Ready, has no merge conflict, and every applicable current-head check has completed successfully.
SQLAlchemy compliance and PyAthena SQLAlchemy/Spark tests are excluded by the unchanged path filters because their trigger paths are unchanged.