🧪 [Add PortabilityExportIncomplete edge case tests] - #12482
Conversation
…_export_photo_manifest Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 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 New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 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".
Git-on-my-level
left a comment
There was a problem hiding this comment.
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 oftest_empty_decoded_base64_retained_inline_image_aborts_portability_exportstill passes{"base64": " "}with no monkeypatch, which raisesbinascii.Errorundervalidate=Trueinstead of reaching theif not decoded:branch, so that draft test would fail. Nothing collects or references the file (pytest's defaultpython_files=test_*.pyskips it, and the.gitignore*.orig.*pattern does not cover a bare.origsuffix). Pleasegit rmit.
Verified good
backend/tests/services/users/test_data_export.py—test_invalid_base64_retained_inline_image_aborts_portability_export(parametrized"===","b","not valid base64!!!") correctly drives the strict-decode failure path inbackend/services/users/data_export.py:219-221: each input raisesbinascii.Errorunderbase64.b64decode(..., validate=True)(leading padding, bad length, invalid characters), asserting the specific"inline image bytes are malformed"message rather than the non-strtype branch.test_empty_decoded_base64_retained_inline_image_aborts_portability_export— monkeypatchingdata_export.base64.b64decodeto returnb""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 theif not decoded:guard atdata_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
left a comment
There was a problem hiding this comment.
Approve only: test-coverage addition, not a bug fix — stays approve-only per policy.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot 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) |
|
Addressed Git-on-my-level + Codex P2: |
…_export_photo_manifest Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
|
Thanks @undivisible - the On One thing needs a maintainer decision before merge: the head commit (bf2e6a4) also regenerates
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 |
🎯 What: Added missing edge case unit tests for error handling in the
_export_photo_manifestfunction for invalidbase64retained inline image bytes.📊 Coverage: Covered two new scenarios:
base64fails the strict mode validation such asb64decode('===', validate=True)where the exception occurs natively frombase64.b64decode.validate=Truethrows an exception for all empty strings/whitespaces natively, amonkeypatchwas utilized to emulate returning an empty decoded byte block to successfully hit theif not decodededge case block.✨ Result: Improved test coverage for
backend/services/users/data_export.pypreventing regression issues for thePortabilityExportIncompletelogic.PR created automatically by Jules for task 3589113620673164881 started by @undivisible
Note
Low Risk
Test-only changes with no production code modifications.
Overview
Adds unit tests for
_export_photo_manifestwhen retained conversation photos carry non-empty inlinebase64that fails strict decode validation or decodes to zero bytes.A parametrized case asserts
PortabilityExportIncompletewith "inline image bytes are malformed" for strings that tripb64decode(..., validate=True). A separate test monkeypatchesbase64.b64decodeto 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.