Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2583253002:
Start adding tests for LevelDBWrapper, and fix a bunch of bugs. (Closed)
|
Created:
4 years ago by Marijn Kruisselbrink Modified:
4 years ago CC:
chromium-reviews, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStart adding tests for LevelDBWrapper, and fix a bunch of bugs.
In particular this fixes:
- loading data from leveldb should strip the prefix from the keys
- writing data to leveldb should preprend the prefix to the keys
- CommitChanges should actually commit changes
BUG=586194
Committed: https://crrev.com/d51c84ce0fd84ac7c813b49981897d0e1ca8a6f7
Cr-Commit-Position: refs/heads/master@{#439849}
Patch Set 1 : nicer #
Total comments: 2
Patch Set 2 : fix destruction order of LevelDBWrapperImpl vs LevelDBDatabasePtr #Patch Set 3 : fix StringPiece conversion #
Messages
Total messages: 37 (29 generated)
The CQ bit was checked by mek@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 mek@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 #1 (id:1) has been deleted
mek@chromium.org changed reviewers: + jam@chromium.org, michaeln@chromium.org
Not exhaustive tests yet (and I'm pretty sure I already found a few other bugs in the still untested code), but a start.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2016/12/19 at 17:57:15, Marijn Kruisselbrink wrote: > Not exhaustive tests yet (and I'm pretty sure I already found a few other bugs in the still untested code), but a start. Hmm, maybe wait with reviewing this. It seems I'll have to fix all those other bugs at the same time, or disable some of the tests... (although strangely locally the SanityCheck browser tests are failing differently, but in all kinds of surprising ways).
The CQ bit was checked by mek@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...
lgtm https://codereview.chromium.org/2583253002/diff/20001/content/browser/leveldb... File content/browser/leveldb_wrapper_impl_unittest.cc (right): https://codereview.chromium.org/2583253002/diff/20001/content/browser/leveldb... content/browser/leveldb_wrapper_impl_unittest.cc:45: explicit MockLevelDBDatabase( nit: no need for explicit
On 2016/12/19 18:45:47, Marijn Kruisselbrink wrote: > On 2016/12/19 at 17:57:15, Marijn Kruisselbrink wrote: > > Not exhaustive tests yet (and I'm pretty sure I already found a few other bugs > in the still untested code), but a start. > > Hmm, maybe wait with reviewing this. It seems I'll have to fix all those other > bugs at the same time, or disable some of the tests... (although strangely > locally the SanityCheck browser tests are failing differently, but in all kinds > of surprising ways). ah saw this after I reviewed, ok let me know when you want me to take another look. Thanks for this patch!
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 mek@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 #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by mek@chromium.org to run a CQ dry run
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) 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...
okay, it's not quite as bad as I thought. In patch 2 I fixed bug in the destruction order of LevelDBWrapperImpl instances relative to the database connection they refer to, which fixed the browsertest failures. And then I had a really stupid bug in my unit test that made it fail on windows. I thought there were more problems revealed by this change, but those were actually already there: The MojoDOMStorageContext browser tests flakily end up with the renderer crashing, which I incorrectly blamed on my changes here at first. In fact the crashing is already happening, it just happens late enough for the test to still be considered a success. So still something that needs fixing (some subtly ordering issues around the various done callbacks, and the "done" callback for GetAll being send over a different pipe from all other done callbacks). So this should be good enough to land now, and I'll work on further tests & fixes for LevelDBWrapper, and trying to figure out the best way to deal with the mojo ordering issues separately. https://codereview.chromium.org/2583253002/diff/20001/content/browser/leveldb... File content/browser/leveldb_wrapper_impl_unittest.cc (right): https://codereview.chromium.org/2583253002/diff/20001/content/browser/leveldb... content/browser/leveldb_wrapper_impl_unittest.cc:45: explicit MockLevelDBDatabase( On 2016/12/19 at 19:19:09, jam wrote: > nit: no need for explicit Why not? It's a single argument constructor.
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 mek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org Link to the patchset: https://codereview.chromium.org/2583253002/#ps170001 (title: "fix StringPiece conversion")
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": 170001, "attempt_start_ts": 1482258116117090,
"parent_rev": "40666feb70c3b304d6c09d56a5a0813c5de3387a", "commit_rev":
"abb23da7de6747261e637d18228648cd9bba4962"}
Message was sent while issue was closed.
Description was changed from ========== Start adding tests for LevelDBWrapper, and fix a bunch of bugs. In particular this fixes: - loading data from leveldb should strip the prefix from the keys - writing data to leveldb should preprend the prefix to the keys - CommitChanges should actually commit changes BUG=586194 ========== to ========== Start adding tests for LevelDBWrapper, and fix a bunch of bugs. In particular this fixes: - loading data from leveldb should strip the prefix from the keys - writing data to leveldb should preprend the prefix to the keys - CommitChanges should actually commit changes BUG=586194 Review-Url: https://codereview.chromium.org/2583253002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== Start adding tests for LevelDBWrapper, and fix a bunch of bugs. In particular this fixes: - loading data from leveldb should strip the prefix from the keys - writing data to leveldb should preprend the prefix to the keys - CommitChanges should actually commit changes BUG=586194 Review-Url: https://codereview.chromium.org/2583253002 ========== to ========== Start adding tests for LevelDBWrapper, and fix a bunch of bugs. In particular this fixes: - loading data from leveldb should strip the prefix from the keys - writing data to leveldb should preprend the prefix to the keys - CommitChanges should actually commit changes BUG=586194 Committed: https://crrev.com/d51c84ce0fd84ac7c813b49981897d0e1ca8a6f7 Cr-Commit-Position: refs/heads/master@{#439849} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d51c84ce0fd84ac7c813b49981897d0e1ca8a6f7 Cr-Commit-Position: refs/heads/master@{#439849} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
