Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2568543004:
[Sync] Enable USS DeviceInfo for bots. (Closed)
Description[Sync] Enable USS DeviceInfo for bots.
As we turn the feature EnableSyncUSSDeviceInfo to handle DeviceInfo
with a USS approach instead of via Directory, we want to have the bots
testing the new code paths.
There were several issues that were fixed before this patch. Here we're
only making two changes to fix failing tests.
1. Python sync server implementation is relaxing its restrictions about
progress marker. It seemed arbitrarily and pedantic for it to treat
empty tokens and non-present tokens differently, so now starts both as
first time syncs. This more accurately reflects how the real server
handles this case.
2. EnableDisableSingleClientTest no longer tests USS types. As we add
more USS types we'll unfortunately have to update this code to exclude
those types as well. There isn't a good USS equivalent to checking for
top level nodes to see if a type has actual sync data. There is no
directory and there are no top level nodes. When USS is completely
rolled out we may just want to remove this test class.
BUG=650725
Committed: https://crrev.com/ba9ef11b0f803870ef935fa1a024f94b7d364944
Cr-Commit-Position: refs/heads/master@{#439870}
Patch Set 1 #Patch Set 2 : Actually adding the bot config. #Patch Set 3 : Rebase, removing fake_server.cc c/p. #Patch Set 4 : Rebase, depending on device info service init move. #Patch Set 5 : Rebase #Patch Set 6 : Rebase. #
Total comments: 2
Patch Set 7 : Rebase and only omit types if uss feature enables them. #Patch Set 8 : Rebase #Patch Set 9 : Actually rebasing this time. #
Depends on Patchset: Messages
Total messages: 51 (42 generated)
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Sync] Enable USS DeviceInfo for bots. BUG=650725 ========== to ========== [Sync] Enable USS DeviceInfo for bots. As we turn the feature EnableSyncUSSDeviceInfo to handle DeviceInfo with a USS approach instead of via Directory, we want to have the bots testing the new code paths. There were several issues that were fixed before this patch. Here we're only making two changes to fix failing tests. 1. Python sync server implementation is relaxing its restrictions about progress marker. It seemed arbitrarily and pedantic for it to treat empty tokens and non-present tokens differently, so now starts both as first time syncs. This more accurately reflects how the real server handles this case. 2. EnableDisableSingleClientTest no longer tests USS types. As we add more USS types we'll unfortunately have to update this code to exclude those types as well. There isn't a good USS equivalent to checking for top level nodes to see if a type has actual sync data. There is no directory and there are no top level nodes. When USS is completely rolled out we may just want to remove this test class. BUG=650725 ==========
Description was changed from ========== [Sync] Enable USS DeviceInfo for bots. As we turn the feature EnableSyncUSSDeviceInfo to handle DeviceInfo with a USS approach instead of via Directory, we want to have the bots testing the new code paths. There were several issues that were fixed before this patch. Here we're only making two changes to fix failing tests. 1. Python sync server implementation is relaxing its restrictions about progress marker. It seemed arbitrarily and pedantic for it to treat empty tokens and non-present tokens differently, so now starts both as first time syncs. This more accurately reflects how the real server handles this case. 2. EnableDisableSingleClientTest no longer tests USS types. As we add more USS types we'll unfortunately have to update this code to exclude those types as well. There isn't a good USS equivalent to checking for top level nodes to see if a type has actual sync data. There is no directory and there are no top level nodes. When USS is completely rolled out we may just want to remove this test class. BUG=650725 ========== to ========== [Sync] Enable USS DeviceInfo for bots. DO_NOT_SUBMIT - the fake_server.cc file needs to be reverted but I only set up a single patch set to depend on and copied fake_server.cc over to this one manually. As we turn the feature EnableSyncUSSDeviceInfo to handle DeviceInfo with a USS approach instead of via Directory, we want to have the bots testing the new code paths. There were several issues that were fixed before this patch. Here we're only making two changes to fix failing tests. 1. Python sync server implementation is relaxing its restrictions about progress marker. It seemed arbitrarily and pedantic for it to treat empty tokens and non-present tokens differently, so now starts both as first time syncs. This more accurately reflects how the real server handles this case. 2. EnableDisableSingleClientTest no longer tests USS types. As we add more USS types we'll unfortunately have to update this code to exclude those types as well. There isn't a good USS equivalent to checking for top level nodes to see if a type has actual sync data. There is no directory and there are no top level nodes. When USS is completely rolled out we may just want to remove this test class. BUG=650725 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Description was changed from ========== [Sync] Enable USS DeviceInfo for bots. DO_NOT_SUBMIT - the fake_server.cc file needs to be reverted but I only set up a single patch set to depend on and copied fake_server.cc over to this one manually. As we turn the feature EnableSyncUSSDeviceInfo to handle DeviceInfo with a USS approach instead of via Directory, we want to have the bots testing the new code paths. There were several issues that were fixed before this patch. Here we're only making two changes to fix failing tests. 1. Python sync server implementation is relaxing its restrictions about progress marker. It seemed arbitrarily and pedantic for it to treat empty tokens and non-present tokens differently, so now starts both as first time syncs. This more accurately reflects how the real server handles this case. 2. EnableDisableSingleClientTest no longer tests USS types. As we add more USS types we'll unfortunately have to update this code to exclude those types as well. There isn't a good USS equivalent to checking for top level nodes to see if a type has actual sync data. There is no directory and there are no top level nodes. When USS is completely rolled out we may just want to remove this test class. BUG=650725 ========== to ========== [Sync] Enable USS DeviceInfo for bots. As we turn the feature EnableSyncUSSDeviceInfo to handle DeviceInfo with a USS approach instead of via Directory, we want to have the bots testing the new code paths. There were several issues that were fixed before this patch. Here we're only making two changes to fix failing tests. 1. Python sync server implementation is relaxing its restrictions about progress marker. It seemed arbitrarily and pedantic for it to treat empty tokens and non-present tokens differently, so now starts both as first time syncs. This more accurately reflects how the real server handles this case. 2. EnableDisableSingleClientTest no longer tests USS types. As we add more USS types we'll unfortunately have to update this code to exclude those types as well. There isn't a good USS equivalent to checking for top level nodes to see if a type has actual sync data. There is no directory and there are no top level nodes. When USS is completely rolled out we may just want to remove this test class. BUG=650725 ==========
skym@chromium.org changed reviewers: + maxbogue@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
skym@chromium.org changed reviewers: + rkaplow@chromium.org
PTAL maxbogue - sync changes rkaplow - fieldtrial_testing_config.json - this file has the same delta as in https://codereview.chromium.org/2549853002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/12/13 00:04:36, skym wrote: > PTAL > > maxbogue - sync changes > rkaplow - fieldtrial_testing_config.json - this file has the same delta as in > https://codereview.chromium.org/2549853002/ Looks like I jumped the gun on posting this change, still broken on windows.
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm well, fttc looks fine, but don't submit if broken on windows
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm https://codereview.chromium.org/2568543004/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/enable_disable_test.cc (right): https://codereview.chromium.org/2568543004/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/enable_disable_test.cc:52: return set; Can't you just do return ModelTypeSet(syncer::DeviceInfo); ? Also, shouldn't it be possible to add types conditionally based on their feature flag? I guess in that case the .Put() structure would work better.
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:140001) has been deleted
https://codereview.chromium.org/2568543004/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/enable_disable_test.cc (right): https://codereview.chromium.org/2568543004/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/enable_disable_test.cc:52: return set; On 2016/12/19 20:55:45, maxbogue wrote: > Can't you just do return ModelTypeSet(syncer::DeviceInfo); ? > > Also, shouldn't it be possible to add types conditionally based on their feature > flag? I guess in that case the .Put() structure would work better. Done.
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by skym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, maxbogue@chromium.org Link to the patchset: https://codereview.chromium.org/2568543004/#ps180001 (title: "Actually rebasing this time.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1482261110927270,
"parent_rev": "f9460b097c26c35972feb3d29214703d652c05b3", "commit_rev":
"4549fe02b7d649600c6b78191ce1a4fe0626bce5"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] Enable USS DeviceInfo for bots. As we turn the feature EnableSyncUSSDeviceInfo to handle DeviceInfo with a USS approach instead of via Directory, we want to have the bots testing the new code paths. There were several issues that were fixed before this patch. Here we're only making two changes to fix failing tests. 1. Python sync server implementation is relaxing its restrictions about progress marker. It seemed arbitrarily and pedantic for it to treat empty tokens and non-present tokens differently, so now starts both as first time syncs. This more accurately reflects how the real server handles this case. 2. EnableDisableSingleClientTest no longer tests USS types. As we add more USS types we'll unfortunately have to update this code to exclude those types as well. There isn't a good USS equivalent to checking for top level nodes to see if a type has actual sync data. There is no directory and there are no top level nodes. When USS is completely rolled out we may just want to remove this test class. BUG=650725 ========== to ========== [Sync] Enable USS DeviceInfo for bots. As we turn the feature EnableSyncUSSDeviceInfo to handle DeviceInfo with a USS approach instead of via Directory, we want to have the bots testing the new code paths. There were several issues that were fixed before this patch. Here we're only making two changes to fix failing tests. 1. Python sync server implementation is relaxing its restrictions about progress marker. It seemed arbitrarily and pedantic for it to treat empty tokens and non-present tokens differently, so now starts both as first time syncs. This more accurately reflects how the real server handles this case. 2. EnableDisableSingleClientTest no longer tests USS types. As we add more USS types we'll unfortunately have to update this code to exclude those types as well. There isn't a good USS equivalent to checking for top level nodes to see if a type has actual sync data. There is no directory and there are no top level nodes. When USS is completely rolled out we may just want to remove this test class. BUG=650725 Review-Url: https://codereview.chromium.org/2568543004 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Enable USS DeviceInfo for bots. As we turn the feature EnableSyncUSSDeviceInfo to handle DeviceInfo with a USS approach instead of via Directory, we want to have the bots testing the new code paths. There were several issues that were fixed before this patch. Here we're only making two changes to fix failing tests. 1. Python sync server implementation is relaxing its restrictions about progress marker. It seemed arbitrarily and pedantic for it to treat empty tokens and non-present tokens differently, so now starts both as first time syncs. This more accurately reflects how the real server handles this case. 2. EnableDisableSingleClientTest no longer tests USS types. As we add more USS types we'll unfortunately have to update this code to exclude those types as well. There isn't a good USS equivalent to checking for top level nodes to see if a type has actual sync data. There is no directory and there are no top level nodes. When USS is completely rolled out we may just want to remove this test class. BUG=650725 Review-Url: https://codereview.chromium.org/2568543004 ========== to ========== [Sync] Enable USS DeviceInfo for bots. As we turn the feature EnableSyncUSSDeviceInfo to handle DeviceInfo with a USS approach instead of via Directory, we want to have the bots testing the new code paths. There were several issues that were fixed before this patch. Here we're only making two changes to fix failing tests. 1. Python sync server implementation is relaxing its restrictions about progress marker. It seemed arbitrarily and pedantic for it to treat empty tokens and non-present tokens differently, so now starts both as first time syncs. This more accurately reflects how the real server handles this case. 2. EnableDisableSingleClientTest no longer tests USS types. As we add more USS types we'll unfortunately have to update this code to exclude those types as well. There isn't a good USS equivalent to checking for top level nodes to see if a type has actual sync data. There is no directory and there are no top level nodes. When USS is completely rolled out we may just want to remove this test class. BUG=650725 Committed: https://crrev.com/ba9ef11b0f803870ef935fa1a024f94b7d364944 Cr-Commit-Position: refs/heads/master@{#439870} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ba9ef11b0f803870ef935fa1a024f94b7d364944 Cr-Commit-Position: refs/heads/master@{#439870} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
