Write integration test to verify that Wallet USS implementation handles gc directives correctly |
|||||||
Issue descriptionThe server currently sends version info based GC directives in order to clear the clients entire sync storage for that datatype. This doesn't work in USS, because the processor only clears the metadata when receiving a GC directive. We want to change the clients behavior for datatypes on USS to always clear the entire client storage when receiving a GC directive that has a version info marker set. This is implemented by making the client tag based processor call MergeSyncData instead of ApplySyncChanges on the bridge when an update comes in that includes the version info directive. This bug is for tracking that we have integration tests that verify this behavior for Wallet
,
Aug 22
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79 commit cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79 Author: Tim Schumann <tschumann@chromium.org> Date: Thu Aug 23 15:30:16 2018 Production-like server behavior for Wallet data in sync integration tests. Change the sync Wallet integration tests to simulate a more realistic server behavior. In particular, provide a realistic version number, GC directives, no client defined unique tags, and non-incremental updates. Wallet data cannot be served by the loopback server (which only supports incremental updates) and shouldn't as that server is used for local sync. Instead, this functionality is added to the FakeServer. Worth noting: Adding time-based values to the progress marker (gc directive's version_watermark) required changes in the way how integration tests verify two clients are in an equal sync-state. Before this was done by comparing the complete progress markers. Verified that when enabling USS for autofill wallet data, the incorrect behavior of dropping cards without a client tag is exercised. Bug: 876308 Change-Id: I9fc8929a508ce5fb62df79a1acf62c65ff98d0fe Reviewed-on: https://chromium-review.googlesource.com/1184821 Commit-Queue: Tim Schumann <tschumann@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#585492} [modify] https://crrev.com/cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79/chrome/browser/sync/test/integration/quiesce_status_change_checker.cc [modify] https://crrev.com/cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc [modify] https://crrev.com/cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79/components/sync/test/fake_server/fake_server.cc [modify] https://crrev.com/cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79/components/sync/test/fake_server/fake_server.h
,
Aug 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0a1056527ca6c83155192d4e51c03a9671d17d49 commit 0a1056527ca6c83155192d4e51c03a9671d17d49 Author: Tim Schumann <tschumann@chromium.org> Date: Mon Aug 27 16:23:35 2018 Add a Wallet integration test covering non-incremental wallet updates. This verified GC directives are properly implemented. In a subsequent CL, we should add a test that verifies empty updates are interpreted correctly. In particular in cases where the client state should not change, e.g. because the server decided to not fetch updates from the backend and signal a "no change" instead. Bug: 876308 Change-Id: I94420353b8aa63f6ec3799d4a44a9c8c89835a97 Reviewed-on: https://chromium-review.googlesource.com/1190202 Commit-Queue: Tim Schumann <tschumann@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#586278} [modify] https://crrev.com/0a1056527ca6c83155192d4e51c03a9671d17d49/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
,
Aug 27
The following test is still missing and needs to be added to fix this bug: Unassigning myself as I'm OOO for a week. A test that verifies empty updates are interpreted correctly. In particular in cases where the client state should not change, e.g. because the server decided to not fetch updates from the backend and signal a "no change" instead.
,
Aug 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a commit f21eb69ffa8b8b5909f5245c4ea49a67c3da673a Author: Florian Uunk <feuunk@chromium.org> Date: Tue Aug 28 15:07:26 2018 Handle version watermark gc directives as full clears. The server currently sends version_watermark based GC directives in order to clear the clients entire sync storage for that datatype. This doesn't work in USS, because the processor only clears the metadata when receiving a GC directive. This means that the data itself is left as-is. This CL changes the expectations on the client when receiving a version_watermark GC directive to always clear the entire client storage. This is implemented in USS by handling an update that includes that GC directive as a full update that replaces any existing client data. We do this by allowing bridges to specify whether they support incremental updates or if they require full data updates. The processor then reports a ModelError when receiving an incremental update for a bridge that doesn't support incremental updates, or when receiving a full update for a bridge that supports incremental updates. When receiving a full update, the processor calls MergeSyncData instead of ApplySyncChanges on the bridge. Bug: 876308 Change-Id: I7fc83e75eb039346866cc6ebd7cf5aa4d9546fdb Reviewed-on: https://chromium-review.googlesource.com/1183902 Commit-Queue: Florian Uunk <feuunk@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#586703} [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.cc [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.h [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge_unittest.cc [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model/fake_model_type_sync_bridge.cc [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model/fake_model_type_sync_bridge.h [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model/model_type_sync_bridge.cc [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model/model_type_sync_bridge.h [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model_impl/client_tag_based_model_type_processor.cc [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model_impl/client_tag_based_model_type_processor.h [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/protocol/sync.proto [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/test/engine/mock_model_type_worker.cc [modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/test/engine/mock_model_type_worker.h
,
Aug 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/539d51ca61501491c455b2847890b0832fe97bd7 commit 539d51ca61501491c455b2847890b0832fe97bd7 Author: Jan Krcal <jkrcal@chromium.org> Date: Wed Aug 29 18:28:16 2018 [USS] Do not expect GC clear all directive for initial sync Before this CL, migration to USS for wallet data USS bridge fails because - this bridge does not support incremental updates, - a clear-all gc directive is expected for every (non-empty) update for such bridges, and - the uss migrator does not provide the directive (as it has no clue about which data types expect which directives). This CL exempts initial updates from this rule and allows them to arrive without any gc directive. Bug: 876308 Change-Id: I2dd91f077ee3602d88e96c5532c3bb910b48c224 Reviewed-on: https://chromium-review.googlesource.com/1194366 Commit-Queue: Jan Krcal <jkrcal@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#587206} [modify] https://crrev.com/539d51ca61501491c455b2847890b0832fe97bd7/components/sync/model_impl/client_tag_based_model_type_processor.cc [modify] https://crrev.com/539d51ca61501491c455b2847890b0832fe97bd7/components/sync/model_impl/client_tag_based_model_type_processor.h [modify] https://crrev.com/539d51ca61501491c455b2847890b0832fe97bd7/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
,
Sep 3
,
Sep 6
hmm... I'm not sure we have complete test coverage for empty wallet updates in the integration test (left a comment in https://chromium-review.googlesource.com/c/chromium/src/+/1193885). Re-opening this bug and assigning to jkrcal@ to double-check.
,
Sep 6
,
Sep 17
Thanks for raising it Tim, I have a CL to review (https://chromium-review.googlesource.com/c/chromium/src/+/1221311) so the bug is in progress :)
,
Sep 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/95077d65ebd9deaf8698285db56cebf3dd611633 commit 95077d65ebd9deaf8698285db56cebf3dd611633 Author: Jan Krcal <jkrcal@chromium.org> Date: Thu Sep 20 06:44:33 2018 [AF USS] Fix the integration test for empty updates This CL refines a previous test and the FakeServer so that it actually tests what it is supposed to test. The CL also does minor polish in the FakeServer code (so that I can understand what is going on). Double-checked by breaking the code. Bug: 876308 Change-Id: I0885647c42ec029db0b76f998840c7378d3496c1 Reviewed-on: https://chromium-review.googlesource.com/1221311 Reviewed-by: Tim Schumann <tschumann@chromium.org> Commit-Queue: Jan Krcal <jkrcal@chromium.org> Cr-Commit-Position: refs/heads/master@{#592702} [modify] https://crrev.com/95077d65ebd9deaf8698285db56cebf3dd611633/chrome/browser/sync/test/integration/quiesce_status_change_checker.cc [modify] https://crrev.com/95077d65ebd9deaf8698285db56cebf3dd611633/components/sync/test/fake_server/fake_server.cc [modify] https://crrev.com/95077d65ebd9deaf8698285db56cebf3dd611633/components/sync/test/fake_server/fake_server.h
,
Sep 20
Hopefully fixed, finally :) |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by feuunk@google.com
, Aug 21