Skip to content

🧪 [testing improvement] Add tests for _export_photo_manifest - #12488

Open
undivisible wants to merge 4 commits into
mainfrom
fix-data-export-testing-6564531978469913391
Open

🧪 [testing improvement] Add tests for _export_photo_manifest#12488
undivisible wants to merge 4 commits into
mainfrom
fix-data-export-testing-6564531978469913391

Conversation

@undivisible

@undivisible undivisible commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

🎯 What: Added missing unit tests for the _export_photo_manifest function in backend/services/users/data_export.py to ensure correct portability metadata generation.
📊 Coverage: Covered edge cases for require_bytes=True including missing references, malformed inline bytes, empty storage objects, and successful extraction of both inline bytes and storage payload.
Result: Improved test coverage and reliability for data export features.


PR created automatically by Jules for task 6564531978469913391 started by @undivisible

Review in cubic


Note

Low Risk
Test-only changes with no runtime or export logic modifications.

Overview
Adds seven unit tests in test_data_export.py for _export_photo_manifest when require_bytes=True (the frame-request / fail-closed path).

They lock in PortabilityExportIncomplete for missing byte references, malformed inline base64 (wrong type or invalid encoding), and empty downloaded storage objects, with download_frame_request_pixels mocked where needed. They also assert successful manifests for valid inline base64 and non-empty storage payloads (bytes_available and bytes_base64).

No production code changes — coverage and regression safety for portability photo manifest behavior only.

Reviewed by Cursor Bugbot for commit b0a2199. Configure here.

…port.py`

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-31T16:55:58.330429Z 198c0a4 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified locally in a worktree — the new coverage itself is solid; one mechanical blocker remains.

What's good (backend/tests/services/users/test_data_export.py):

  • Each new test maps onto a real branch of _export_photo_manifest in backend/services/users/data_export.py: the missing-reference raise with require_bytes=True ("retained image bytes reference is missing or malformed"), both malformed-inline paths (non-string base64, invalid base64), the valid-inline path (bytes_available / bytes_base64), and both storage paths through download_frame_request_pixels — including the empty-payload raise ("retained image object is empty") and assert_called_once_with("uid1", "storage-1") argument checks.
  • All 6 new tests pass, and the full file is 39/39 green locally against the current production function. The require_bytes=False legacy path was already covered by test_legacy_conversation_photo_without_any_bytes_reference_exports_metadata, so there's no gap there.

Blocking: the Formatting check fails, and it's a one-line fix.
The repo pins Black 26.5.1 via scripts/backend-python-format (line-length 120). Running the identical check locally, the only reformat it wants is a second blank line before the first new test:

     assert manifest["bytes_unavailable_reason"] == "no_retained_bytes_reference"
 
 
+
+def test_export_photo_manifest_require_bytes_missing_reference():

(Two blank lines before a top-level def.) Adding that blank line — or running scripts/backend-python-format --write backend/tests/services/users/test_data_export.py — should turn the check green; nothing else in the added code needs reformatting.

Once formatting is fixed this looks ready — no further maintainer judgment needed beyond that.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@Git-on-my-level Git-on-my-level added the backend Backend Task (python) label Aug 31, 2026

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approve only: test-coverage addition, not a bug fix — stays approve-only per policy.

@cursor

cursor Bot commented Sep 1, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_f07426b6-c62f-46ca-bf65-18bcc5b0e867)

@undivisible

Copy link
Copy Markdown
Collaborator Author

Addressed Git-on-my-level Black CR: added the second blank line before test_export_photo_manifest_require_bytes_missing_reference. Pushed in bddb8f2.

…port.py`

Failure-Class: none

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
@cursor

cursor Bot commented Sep 1, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_dfcf3b39-c695-44fd-a19f-327fa40eefcf)

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Checked the new head (0941f722): the coverage itself is unchanged and still solid — all 39 tests in backend/tests/services/users/test_data_export.py pass locally, including the six new _export_photo_manifest tests.

Heads-up: your formatting fix from bddb8f2abf was reverted by the latest push. bddb8f2abf added the second blank line before test_export_photo_manifest_require_bytes_missing_reference (exactly what the earlier review asked for), but 0941f722 — which reuses the original commit message — removes that blank line again. That's why Formatting is back to failing with would reformat backend/tests/services/users/test_data_export.py. Reproduced locally with the repo-pinned Black 26.5.1 (line-length 120): that single blank line is the only delta. Looks like automation churn rather than intent — re-adding it (or re-applying bddb8f2abf) should turn the check green.

On the other two red checks: Hygiene and PR Metadata Preflight both fail only on firestore-query-coverage (baseline 102/62 vs inventory 113/73), which also fails on main's own latest run (run 33526472934). This PR doesn't touch any Firestore queries, so that failure isn't coming from this change.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

Formatting CI fails because Black 26.5.1 requires two blank lines
between top-level test functions. Jules' later push dropped the
blank line added in bddb8f2; restore it so the check stays green.

Co-authored-by: Max Carter 祁明思 <max@tsc.hk>
@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_b725e525-5875-472f-9e59-db49688ec219)

@Git-on-my-level
Git-on-my-level dismissed their stale review September 2, 2026 15:49

Dismissing: the only blocking concern (missing second blank line before test_export_photo_manifest_require_bytes_missing_reference, Black 26.5.1) was restored in b0a2199 — pinned formatting check and the full test file pass on the current head.

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified on the new head (b0a21991) — the last blocker from the earlier review is resolved.

What I checked (backend/tests/services/users/test_data_export.py, +75):

  • All six new tests map onto real branches of _export_photo_manifest in backend/services/users/data_export.py: the require_bytes=True fail-closed raise ("retained image bytes reference is missing or malformed"), both malformed-inline paths (non-string base64, invalid base64), the valid-inline bytes_available/bytes_base64 passthrough, and both download_frame_request_pixels storage paths — including the empty-payload raise ("retained image object is empty") and the assert_called_once_with("uid1", "storage-1") argument checks. Storage access is stubbed via monkeypatch.setattr on the module seam, so nothing reaches external services.
  • Local run: 39/39 tests in the file pass on this head against the current production function.
  • Formatting: the second blank line before test_export_photo_manifest_require_bytes_missing_reference — added in bddb8f2abf, dropped by the 0941f722 rebase — is back in b0a21991. scripts/backend-python-format --check passes locally, matching the green Formatting check. The earlier Hygiene/Preflight reds were the firestore-query-coverage baseline drift that also failed on main, unrelated to this change.

Test-coverage addition only, no production changes. Approving, and dismissing the earlier changes-requested review since its formatting concern is fixed on this head. Thanks for the quick turnaround across the rebase churn.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

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

Labels

backend Backend Task (python)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants