fix: handle full collection sync - #160
robertn702 wants to merge 2 commits into
Conversation
e9e8907 to
c5f071b
Compare
| if output.server_message: | ||
| console.print(output.server_message) | ||
|
|
||
| if output.required != output.NO_CHANGES: |
There was a problem hiding this comment.
Should we add and output.required != output.NORMAL_SYNC?
There was a problem hiding this comment.
Or does output.required == output.NORMAL_SYNC mean that a normal sync is required, which seems like a scenario that should not really happen?
| return | ||
| console.print("[red]Sync requires an Anki profile.") | ||
| raise Abort() | ||
|
|
||
| hkey = self._profile.get("syncKey") | ||
| if not hkey: | ||
| return | ||
| console.print("[red]Sync requires a sync key.") | ||
| raise Abort() |
There was a problem hiding this comment.
This is not quite safe and leads to a lot of failed tests and unexpected behaviour. With auto_sync on and no sync key, a successful mutating command now ends in "Aborted!" and exit 1 after the work is done. Erroring is right for an explicit apy sync, but for auto sync path should only warn and skip.
There was a problem hiding this comment.
Notice that this is an interesting case where the tests fail on my end because of my own config. We should add a conftest.py file that specifes a non-existing config file. I can do that myself later.
To reproduce this problem, you need to ensure that you have auto_sync enabled in your config.
|
|
||
| if output.new_endpoint: | ||
| auth.endpoint = output.new_endpoint | ||
| self._profile["currentSyncUrl"] = output.new_endpoint |
There was a problem hiding this comment.
I don't think this is necessary. It won't be persisted, so I think it ends up having no real effect?
| if output.required != output.NO_CHANGES: | ||
| if output.required == output.FULL_DOWNLOAD: | ||
| upload = False | ||
| confirmed = console.confirm( |
There was a problem hiding this comment.
The confirm dialogs don't work, because they're hidden behind the with Progress on line 190. Needs progress.stop() around the prompt.
lervag
left a comment
lervag
left a comment
There was a problem hiding this comment.
Thanks, I appreciate the PR, it addresses a real bug!
On tests/test_sync.py: Thanks for adding these. I'd like to redirect the effort though, because as written they don't constrain much.
The tests construct the object under test with Anki.__new__(Anki) and assign _profile and col by hand, then assert exact call sequences against a hand-written FakeCollection. That describes the implementation rather than the behaviour: assert collection.operations == [("normal", True), ("backup", ...), ("close", None), ...] breaks on any harmless reordering, while real defects pass straight through. There are two real defects in this PR that passes all seven tests:
-
The confirmation prompt is never visible. It's issued inside the live Progress block, so rich erases it on the next refresh (~100ms). Under a pty you get a spinner and an apparently hung program. Pressing Enter takes the default and aborts. The tests can't see this because
console.confirmis monkeypatched away. That is, the test substitutes exactly the part that's broken. -
sync()now raisesAbortwith no profile or sync key, which__exit__reaches viaauto_sync. With"auto_sync": truein~/.config/apy/apy.json, this branch has 25 failed tests. CI missed it because CI has noapy.jsonconfig. And yes, this is a pre-existing gap in the test setup which I'll fix separately.
What I'd find more valuable:
- Keep the behavioural tests and drop the sequence ones.
test_sync_without_key_fails_instead_of_reporting_successandtest_sync_conflict_can_be_cancelledencode the actual promise of #159 and are worth having. For the rest, the membership style you already used intest_sync_uploads_when_only_full_upload_is_allowed(assert ("full", ...) in collection.operations) says what matters without freezing the call order. - For real coverage, anki ships a sync server:
SYNC_BASE=... SYNC_USER1=user:pass python -m anki.syncserver. I checked, it starts and listens on a local port. A fixture that spawns it would let you drive the exact scenario in the issue through the real backend, with no fake to keep in sync with anki's API.
I'll let the second "real coverage" item be optional here, since I can understand it may feel like a larger task. But if you want, feel free to address it.
|
@lervag Thanks for the thorough review! I'll get around to updating the PR in the next few days. |
I see you've pushed some updates now; let me know when you want me to take another look. And don't hesitate to ask for input or assistance! |
2 participants
Summary
Safety
Full uploads and downloads require confirmation. Downloads create and await a local backup before replacing the collection. The collection is reopened after a failed full transfer and closed if automatic sync aborts.
Tests
uv run ruff format --checkuv run ruff checkuv run pyrefly check --baseline pyrefly-baseline.jsonuv run pytest(38 passed)git diff --checkCloses #159