Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
213433002:
Migrate old Shortcuts DB data to conform to new type values (Closed)
|
Created:
6 years, 9 months ago by Anuj Modified:
6 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, browser-components-watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMigrate old Shortcuts DB data to conform to new type values
BUG=339270
TBR=brettw@chromium.org,shess@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260476
Patch Set 1 : Ready for Review #
Total comments: 9
Patch Set 2 : Addressed comments #Patch Set 3 : Added DCHECKs for debugging #Patch Set 4 : Fix DCHECKs #Patch Set 5 : Make android happy #Patch Set 6 : Delete temp dir #Patch Set 7 : Work around Windows #
Messages
Total messages: 45 (0 generated)
Looks like deleting the Shortcuts.v1.sql in the same CL was the problem. Please review.
https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... File chrome/browser/history/shortcuts_database_unittest.cc (right): https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:231: base::FilePath db_path(temp_dir.path().AppendASCII("TestShortcuts1.db")); The conflict in the db files created by two tests was the reason for test failure.
- Tested on android and chromeos build on linux. - Will delete Shortcuts.v1.sql separately.
https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... File chrome/browser/history/shortcuts_database_unittest.cc (right): https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:224: // Open the v0 test file and use it to create a test database in a temp dir. Nit: v0 -> pre-v0 https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:231: base::FilePath db_path(temp_dir.path().AppendASCII("TestShortcuts1.db")); On 2014/03/27 01:56:40, Anuj wrote: > The conflict in the db files created by two tests was the reason for test > failure. How is that possible? They're both running CreateUniqueTempDir(), so they should be writing into separate directories, shouldn't they? https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:267: // Open the v1 test file and use it to create a test database in a temp dir. Nit: v1 -> v0
https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... File chrome/browser/history/shortcuts_database_unittest.cc (right): https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:224: // Open the v0 test file and use it to create a test database in a temp dir. On 2014/03/27 20:59:25, Peter Kasting wrote: > Nit: v0 -> pre-v0 Done. https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:231: base::FilePath db_path(temp_dir.path().AppendASCII("TestShortcuts1.db")); On 2014/03/27 20:59:25, Peter Kasting wrote: > On 2014/03/27 01:56:40, Anuj wrote: > > The conflict in the db files created by two tests was the reason for test > > failure. > > How is that possible? They're both running CreateUniqueTempDir(), so they > should be writing into separate directories, shouldn't they? I am not sure. But in my dev setup, the test failed when the file name was same, and succeeded when the file names were different. The error originated on call to CreateDatabaseFromSQL. I will investigate this further in the next CL where I delete the v1.sql https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:267: // Open the v1 test file and use it to create a test database in a temp dir. On 2014/03/27 20:59:25, Peter Kasting wrote: > Nit: v1 -> v0 Done.
https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... File chrome/browser/history/shortcuts_database_unittest.cc (right): https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:231: base::FilePath db_path(temp_dir.path().AppendASCII("TestShortcuts1.db")); On 2014/03/27 21:46:10, Anuj wrote: > I will investigate this further in the next CL where I delete the v1.sql :/ OK, I guess... I'd like to understand it now, because this doesn't seem possible -- it seems like this must indicate some clear bug in the code somewhere -- but I realize you're anxious to get this in, and it doesn't need to block landing.
https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... File chrome/browser/history/shortcuts_database_unittest.cc (right): https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:231: base::FilePath db_path(temp_dir.path().AppendASCII("TestShortcuts1.db")); On 2014/03/27 21:55:28, Peter Kasting wrote: > On 2014/03/27 21:46:10, Anuj wrote: > > I will investigate this further in the next CL where I delete the v1.sql > > :/ OK, I guess... I'd like to understand it now, because this doesn't seem > possible -- it seems like this must indicate some clear bug in the code > somewhere -- but I realize you're anxious to get this in, and it doesn't need to > block landing. So are you okay with this? Or do you want to block this until I figure out what is broken only in android_dbg environment?
On 2014/03/27 22:19:05, Anuj wrote: > https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... > File chrome/browser/history/shortcuts_database_unittest.cc (right): > > https://codereview.chromium.org/213433002/diff/110001/chrome/browser/history/... > chrome/browser/history/shortcuts_database_unittest.cc:231: base::FilePath > db_path(temp_dir.path().AppendASCII("TestShortcuts1.db")); > On 2014/03/27 21:55:28, Peter Kasting wrote: > > On 2014/03/27 21:46:10, Anuj wrote: > > > I will investigate this further in the next CL where I delete the v1.sql > > > > :/ OK, I guess... I'd like to understand it now, because this doesn't seem > > possible -- it seems like this must indicate some clear bug in the code > > somewhere -- but I realize you're anxious to get this in, and it doesn't need > to > > block landing. > > So are you okay with this? Or do you want to block this until I figure out what > is broken only in android_dbg environment? Sorry, I meant to say LGTM.
> > Sorry, I meant to say LGTM. NP. I was confused. Thanks.
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/213433002/130001
The CQ bit was unchecked by skanuj@chromium.org
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/213433002/130001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was unchecked by skanuj@chromium.org
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/213433002/130001
The CQ bit was checked by skanuj@chromium.org
The CQ bit was unchecked by skanuj@chromium.org
The CQ bit was checked by skanuj@chromium.org
The CQ bit was unchecked by skanuj@chromium.org
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/213433002/150001
The CQ bit was unchecked by skanuj@chromium.org
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/213433002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/213433002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/213433002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/213433002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/213433002/230001
Message was sent while issue was closed.
Change committed as 260476 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
