Skip to content

🧪 [Add PortabilityExportIncomplete edge case tests] - #12482

Open
undivisible wants to merge 3 commits into
mainfrom
test/portability-export-incomplete-tests-3589113620673164881
Open

🧪 [Add PortabilityExportIncomplete edge case tests]#12482
undivisible wants to merge 3 commits into
mainfrom
test/portability-export-incomplete-tests-3589113620673164881

Conversation

@undivisible

@undivisible undivisible commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

🎯 What: Added missing edge case unit tests for error handling in the _export_photo_manifest function for invalid base64 retained inline image bytes.

📊 Coverage: Covered two new scenarios:

  1. When base64 fails the strict mode validation such as b64decode('===', validate=True) where the exception occurs natively from base64.b64decode.
  2. When the decoded base64 inline image evaluates to an empty string. Since validate=True throws an exception for all empty strings/whitespaces natively, a monkeypatch was utilized to emulate returning an empty decoded byte block to successfully hit the if not decoded edge case block.

Result: Improved test coverage for backend/services/users/data_export.py preventing regression issues for the PortabilityExportIncomplete logic.


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

Review in cubic


Note

Low Risk
Test-only changes with no production code modifications.

Overview
Adds unit tests for _export_photo_manifest when retained conversation photos carry non-empty inline base64 that fails strict decode validation or decodes to zero bytes.

A parametrized case asserts PortabilityExportIncomplete with "inline image bytes are malformed" for strings that trip b64decode(..., validate=True). A separate test monkeypatches base64.b64decode to return empty bytes so the "inline image bytes are empty" path is exercised without relying on inputs that validate=True already rejects.

Reviewed by Cursor Bugbot for commit f566a6c. Configure here.

…_export_photo_manifest

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:50:42.450546Z 45ebe38 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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 45ebe38417

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread backend/tests/services/users/test_data_export.py.orig Outdated
@Git-on-my-level Git-on-my-level added the backend Backend Task (python) label Aug 31, 2026

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

Thanks @undivisible — the new coverage itself is correct and useful; one cleanup is needed before merge.

Blocking

  • backend/tests/services/users/test_data_export.py.orig (+748) is an editor/merge leftover committed alongside the real change. It is a near-complete stale draft of the test module — its copy of test_empty_decoded_base64_retained_inline_image_aborts_portability_export still passes {"base64": " "} with no monkeypatch, which raises binascii.Error under validate=True instead of reaching the if not decoded: branch, so that draft test would fail. Nothing collects or references the file (pytest's default python_files=test_*.py skips it, and the .gitignore *.orig.* pattern does not cover a bare .orig suffix). Please git rm it.

Verified good

  • backend/tests/services/users/test_data_export.pytest_invalid_base64_retained_inline_image_aborts_portability_export (parametrized "===", "b", "not valid base64!!!") correctly drives the strict-decode failure path in backend/services/users/data_export.py:219-221: each input raises binascii.Error under base64.b64decode(..., validate=True) (leading padding, bad length, invalid characters), asserting the specific "inline image bytes are malformed" message rather than the non-str type branch.
  • test_empty_decoded_base64_retained_inline_image_aborts_portability_export — monkeypatching data_export.base64.b64decode to return b"" is the right seam: every strictly-invalid string (including whitespace-only) raises during decode rather than decoding to empty, so this is the only practical way to reach the if not decoded: guard at data_export.py:222-223. The PR description explains this correctly.
  • Ran the file locally in a worktree of this head: 37 passed (33 pre-existing + 4 new), including the two neighboring pre-existing tests.

Minor nit, no change requested: the new "malformed" parametrization overlaps the existing test_malformed_retained_inline_image_aborts_portability_export ("not-base64!", 123), but it pins the more specific are malformed message and distinct b64decode error modes, so keeping both is reasonable.

Once the .orig file is dropped this is a clear improvement to the PortabilityExportIncomplete regression coverage — the test additions themselves are merge-ready.


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

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

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@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_a33dea13-9dbe-41f6-8a51-0f2988c68d17)

@undivisible

Copy link
Copy Markdown
Collaborator Author

Addressed Git-on-my-level + Codex P2: git rm backend/tests/services/users/test_data_export.py.orig. Pushed in f566a6c.

…_export_photo_manifest

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

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@Git-on-my-level
Git-on-my-level dismissed their stale review September 1, 2026 18:00

Resolved on f566a6c: the committed backend/tests/services/users/test_data_export.py.orig editor leftover was removed; the file is no longer in the tree. Remaining feedback on the current head concerns the separate Firestore query-coverage baseline regeneration, tracked in the PR discussion.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks @undivisible - the test_data_export.py.orig leftover from the earlier review is cleanly removed in f566a6c, and the test additions themselves remain solid.

On backend/tests/services/users/test_data_export.py (+21): the parametrized test_invalid_base64_retained_inline_image_aborts_portability_export cases ("===", "b", "not valid base64!!!") each raise inside base64.b64decode(..., validate=True) and hit the except -> "inline image bytes are malformed" branch at backend/services/users/data_export.py:218-221, and the monkeypatched-empty-decode test remains the only practical way to reach the if not decoded: -> "inline image bytes are empty" guard at data_export.py:222-223. Both pin the specific error messages rather than the non-str branch - good regression coverage. (The file is unchanged since the prior automated review's worktree run: 37 passed.)

One thing needs a maintainer decision before merge: the head commit (bf2e6a4) also regenerates backend/scripts/firestore_query_coverage_baseline.json (eligible_serving 70->113, registered_serving 24->73, six stale raw fingerprints dropped), which is outside the PR's stated test-only scope, is not mentioned in the description, and reuses the first commit's message verbatim. Context, since it is not obvious from the diff: Repo Checks (Hygiene -> firestore-query-coverage) failed on f566a6c with baseline registered_serving is 62, current inventory reports 73 because #12126 refreshed the baseline and added a freshness guard on main after this branch forked from it. The new commit regenerates the baseline exactly as the CI error instructs - I reproduced it independently (--format baseline on this tree yields 113/73 with the identical 37 raw fingerprints; --check-ratchet exits 0; the floor moves up and no raw/unsupported shape is newly permitted). Two wrinkles for a maintainer:

  1. GitHub currently reports this PR as conflicting with main (both branches touched the baseline JSON), so it needs a rebase - and how that conflict is resolved effectively decides main's coverage floor.
  2. The regenerated 113/73 matches what CI's own runner scans on this tree, but differs from what chore(backend): keep the query-coverage baseline honest about itself #12126 committed for main (102/62). The inventory the scanner reports appears to vary between environments, and main's committed baseline fails its own freshness check under a fresh scan - worth a look independent of this PR.

Marking for maintainer review on whether the baseline refresh belongs here or in a dedicated chore PR. @undivisible - could you add a line to the PR description (and ideally reword the head commit message) noting the baseline regeneration and the CI failure that prompted it?

by AI on behalf of David Zhang

@Git-on-my-level Git-on-my-level added needs-maintainer-review Needs a human maintainer to sign off before merge workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior labels Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend Backend Task (python) needs-maintainer-review Needs a human maintainer to sign off before merge workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants