Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2448313002:
Adding unit tests for PreviewsOptOutStoreSQL (Closed)
DescriptionAdding unit tests for PreviewsOptOutStoreSQL
This tests the behavior of the SQL store backing previews opt outs and
verifeis state is maintained correctly and errors are handled correctly.
BUG=654464
Committed: https://crrev.com/bdd23f4cc8ff26cae9a28f17f7ed762f99ae3c94
Cr-Commit-Position: refs/heads/master@{#430341}
Patch Set 1 #Patch Set 2 : ios issue #
Total comments: 4
Patch Set 3 : tbansal comments #Patch Set 4 : rebase #Patch Set 5 : histogram in tests #Patch Set 6 : size_t case to int #
Total comments: 16
Patch Set 7 : tbansal #
Dependent Patchsets: Messages
Total messages: 57 (46 generated)
The CQ bit was checked by ryansturm@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 ryansturm@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 ryansturm@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by ryansturm@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by ryansturm@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 #2 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: PTAL
On 2016/10/26 21:15:07, Ryan Sturm wrote: > tbansal: PTAL "verifeis" typo in CL description
https://codereview.chromium.org/2448313002/diff/100001/components/previews/co... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2448313002/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.h:47: size_t OptOutRecordsSizeForTesting(); const method. https://codereview.chromium.org/2448313002/diff/100001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2448313002/diff/100001/components/previews/co... components/previews/core/previews_experiments.cc:48: // Command line switch to change the previews per row DB size. why is there a need to set this using command line switches? Is it possible to add a virtual method, and override in tests?
The CQ bit was checked by ryansturm@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ryansturm@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by ryansturm@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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ryansturm@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
tbansal: ptal https://codereview.chromium.org/2448313002/diff/100001/components/previews/co... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2448313002/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.h:47: size_t OptOutRecordsSizeForTesting(); On 2016/10/28 22:07:14, tbansal1 wrote: > const method. Done. https://codereview.chromium.org/2448313002/diff/100001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2448313002/diff/100001/components/previews/co... components/previews/core/previews_experiments.cc:48: // Command line switch to change the previews per row DB size. On 2016/10/28 22:07:14, tbansal1 wrote: > why is there a need to set this using command line switches? > Is it possible to add a virtual method, and override in tests? I moved this to previews_opt_out_store_sql.cc I want to avoid injecting these values into the constructor, and I don't think changing static/anonymous methods to class methods or plumbing these values down is a better approach.
mostly nits. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:39: // opt out store. May be add a comment that says that the limit is enforced at the time of preview insertion. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:49: // store. Can you add a comment that says that the limit is enforced when the database is loaded next time, so it is possible for the database to temporarily grow beyond this size. This is important because this is currently a bit unintuitive. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... File components/previews/core/previews_opt_out_store_sql_unittest.cc (right): https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:138: EXPECT_EQ(now, iter->second->most_recent_opt_out_time().value()); why not check other fields too e.g., hostname? https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:210: histogram_tester_.ExpectUniqueSample("Previews.OptOut.DBRowCount", 0, 1); Put this in a for loop of size |row_limit|? https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:228: DestroyStore(); Add: EXPECT_EQ(row_limit, iter->second->OptOutRecordsSizeForTesting()); before destroying? https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:230: // Reload and test for persistence missing period at end. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:241: EXPECT_EQ(row_limit, iter->second->OptOutRecordsSizeForTesting()); Is it possible to check that the older entries were deleted, and not the recent ones?
mostly nits. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:39: // opt out store. May be add a comment that says that the limit is enforced at the time of preview insertion. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:49: // store. Can you add a comment that says that the limit is enforced when the database is loaded next time, so it is possible for the database to temporarily grow beyond this size. This is important because this is currently a bit unintuitive. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... File components/previews/core/previews_opt_out_store_sql_unittest.cc (right): https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:138: EXPECT_EQ(now, iter->second->most_recent_opt_out_time().value()); why not check other fields too e.g., hostname? https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:210: histogram_tester_.ExpectUniqueSample("Previews.OptOut.DBRowCount", 0, 1); Put this in a for loop of size |row_limit|? https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:228: DestroyStore(); Add: EXPECT_EQ(row_limit, iter->second->OptOutRecordsSizeForTesting()); before destroying? https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:230: // Reload and test for persistence missing period at end. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:241: EXPECT_EQ(row_limit, iter->second->OptOutRecordsSizeForTesting()); Is it possible to check that the older entries were deleted, and not the recent ones?
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:39: // opt out store. On 2016/11/07 17:10:26, tbansal1 wrote: > May be add a comment that says that the limit is enforced at the time of preview > insertion. Done. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:49: // store. On 2016/11/07 17:10:26, tbansal1 wrote: > Can you add a comment that says that the limit is enforced when the database is > loaded next time, so it is possible for the database to temporarily grow beyond > this size. This is important because this is currently a bit unintuitive. Done. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... File components/previews/core/previews_opt_out_store_sql_unittest.cc (right): https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:138: EXPECT_EQ(now, iter->second->most_recent_opt_out_time().value()); On 2016/11/07 17:10:26, tbansal1 wrote: > why not check other fields too e.g., hostname? hostname isn't stored in the item and this item is found via black_list_map_->find(test_host); https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:210: histogram_tester_.ExpectUniqueSample("Previews.OptOut.DBRowCount", 0, 1); On 2016/11/07 17:10:27, tbansal1 wrote: > Put this in a for loop of size |row_limit|? Done. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:228: DestroyStore(); On 2016/11/07 17:10:26, tbansal1 wrote: > Add: > EXPECT_EQ(row_limit, iter->second->OptOutRecordsSizeForTesting()); > before destroying? There's no way to get the iter without calling CreateAndLoad again. The DB is only read at start up and this test does not manage its own map. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:230: // Reload and test for persistence On 2016/11/07 17:10:26, tbansal1 wrote: > missing period at end. Done. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:241: EXPECT_EQ(row_limit, iter->second->OptOutRecordsSizeForTesting()); On 2016/11/07 17:10:26, tbansal1 wrote: > Is it possible to check that the older entries were deleted, and not the recent > ones? The combination of line 239/240 and 245 should guarantee we have the opt out that came second and the non-opt out that came third (meaning the first opt-out was evicted).
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... File components/previews/core/previews_opt_out_store_sql_unittest.cc (right): https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:138: EXPECT_EQ(now, iter->second->most_recent_opt_out_time().value()); On 2016/11/07 18:24:29, Ryan Sturm wrote: > On 2016/11/07 17:10:26, tbansal1 wrote: > > why not check other fields too e.g., hostname? > > hostname isn't stored in the item and this item is found via > black_list_map_->find(test_host); Acknowledged. https://codereview.chromium.org/2448313002/diff/180001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:241: EXPECT_EQ(row_limit, iter->second->OptOutRecordsSizeForTesting()); On 2016/11/07 18:24:29, Ryan Sturm wrote: > On 2016/11/07 17:10:26, tbansal1 wrote: > > Is it possible to check that the older entries were deleted, and not the > recent > > ones? > > The combination of line 239/240 and 245 should guarantee we have the opt out > that came second and the non-opt out that came third (meaning the first opt-out > was evicted). Acknowledged.
The CQ bit was unchecked by ryansturm@chromium.org
The CQ bit was checked by ryansturm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Adding unit tests for PreviewsOptOutStoreSQL This tests the behavior of the SQL store backing previews opt outs and verifeis state is maintained correctly and errors are handled correctly. BUG=654464 ========== to ========== Adding unit tests for PreviewsOptOutStoreSQL This tests the behavior of the SQL store backing previews opt outs and verifeis state is maintained correctly and errors are handled correctly. BUG=654464 Committed: https://crrev.com/bdd23f4cc8ff26cae9a28f17f7ed762f99ae3c94 Cr-Commit-Position: refs/heads/master@{#430341} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/bdd23f4cc8ff26cae9a28f17f7ed762f99ae3c94 Cr-Commit-Position: refs/heads/master@{#430341} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
