Skip to content

Report "Checksum matches" only after download verification; strengthen unit tests - #1197

Draft
sophiecarreras wants to merge 1 commit into
masterfrom
review/cli-fixes-and-tests
Draft

sophiecarreras wants to merge 1 commit into
masterfrom
review/cli-fixes-and-tests

Conversation

@sophiecarreras

Copy link
Copy Markdown
Contributor

Summary

  • file download printed "Checksum matches" before the download completed, and even with --skip-hash-verification. It now prints after the data is saved and verified, and never when verification is skipped.
  • The unit-test harness mock _TqdmCloser.__exit__ returned a truthy MagicMock, which silently swallowed exceptions from any command using a progress bar. It now returns None. Two tests that depended on the swallowing now suppress the expected KeyError explicitly.
  • Adds or strengthens unit tests for: rm by file id, incremental upload, destination SSE on the wire, unbound-stream --part-size, sync thread options, sdkharness health and conformance negative paths, camelCase rejection in v4, download-file-by-name flags, rm progress, arg-parser and sdkharness file-lifecycle coverage.

Test plan

  • pytest test/unit: 621 passed, 6 failed. The 6 are in test_autocomplete_cache.py and fail identically on the unmodified base in my environment; I did not investigate the cause.
  • ruff check and ruff format --check pass on the changed files. Integration tests (need B2 credentials) were not run.
  • The findings came from an automated review and have not been confirmed by a maintainer yet; please treat the fixes as proposals.

🤖 Generated with Claude Code

…ngthen unit tests

- `file download` prints "Checksum matches" after the data has been saved
  and verified, and not at all with --skip-hash-verification (previously it
  was printed before the download and even when verification was skipped).
- Unit-test harness: the autouse _TqdmCloser.__exit__ mock returned a truthy
  MagicMock, which swallowed every exception raised by a command that uses a
  progress bar. It now returns None; two tests that relied on the swallowing
  suppress the expected KeyError explicitly.
- New or strengthened unit tests: rm by file id (hide marker and version),
  incremental upload, --destination-server-side-encryption on the wire,
  unbound-stream --part-size, sync thread options, sdkharness health and
  conformance negative paths, camelCase rejection in v4, download-file-by-name
  flags, rm progress, arg-parser and sdkharness file-lifecycle coverage.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@sophiecarreras

Copy link
Copy Markdown
Contributor Author

Review request from Sofia (drafted with Claude). Nothing in this review has been checked by a human yet; the numbers come from an agent review of whole main. Please verify, in this order:

  1. For each new or changed test, revert the production line it covers (or the named fix) and confirm the test fails. A test that still passes is a finding.
  2. No production behavior changed beyond what the description says.
  3. The decision or check listed for this PR below.
    Reply on the linked issue if a finding is a real bug, not a bug, or already known.

This PR: Snyk Code reports one new issue that is not yet identified (someone with Snyk access needs to open it). 6 to 8 tests in test_autocomplete_cache.py fail on the unmodified base too, cause unknown.

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.

1 participant