Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
334303002:
Using a mock LevelDBTransaction for corruption tests. (Closed)
DescriptionUsing a mock LevelDBTransaction for corruption tests.
Was previously flushing the actual LevelDB table on disk and then corrupting.
This approach is more loosely coupled with LevelDB disk format and lays the
groundwork for future tests (and refactoring) to not rely on an actual
corrupted db.
Also reduced the number of test objects written to the db to 5.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278838
Patch Set 1 #
Total comments: 11
Patch Set 2 : Reduced scope of s_factory, "num" -> "instNum", etc. #
Total comments: 5
Patch Set 3 : '"' -> "\"" #Patch Set 4 : Factored the new browser IDB content test objects into own file. #
Total comments: 8
Patch Set 5 : Combined VLOG and NOTREACHED #
Total comments: 13
Patch Set 6 : Renamed IndexedDBBrowserTestClassFactory, using url::ExtractQueryKeyValue, and misc. #Patch Set 7 : LevelDBTraceTansaction::s_class_name is now static #
Messages
Total messages: 22 (0 generated)
I'm still reading this, but in terms of general philosophy I have mixed feelings. We want to be able to test the higher-level code without having to worry too much about the actual disk format--sure. But it's also good to know that particular real corruptions of leveldb are detected properly. That's more of a leveldb test than an indexeddb test, but there's value in having it close to our code, since we're the ones who care about it. Perhaps we should have both test types?
On 2014/06/16 23:12:34, ericu wrote: > I'm still reading this, but in terms of general philosophy I have mixed > feelings. We want to be able to test the higher-level code without having to > worry too much about the actual disk format--sure. But it's also good to know > that particular real corruptions of leveldb are detected properly. That's more > of a leveldb test than an indexeddb test, but there's value in having it close > to our code, since we're the ones who care about it. > > Perhaps we should have both test types? I agree that we shouldn't be testing that LevelDB properly detects and reports corruption - those tests belong in the LevelDB project. We should be testing that we properly handle LevelDB failures. Basically if any LevelDB call returns an error code, that we deal with it properly. I don't necessarily want to prevent tests that rely on an actual corrupted database from being written. The reason I took this approach was that I needed to test a failure to leveldb::Db::Write(), and after several different approaches was never able to get the db into a state where I could open it up, but then get Write() to fail.
https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:215: test_class_factory_(new IndexedDBBrowserTestClassFactory) {} Is test_class_factory_ actually used somewhere? If so, should it be a scoped_ptr instead of raw? https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:228: IndexedDBClassFactory::SetIndexedDBClassFactoryGetter(NULL); Given this cleanup, do we need s_factory to be a static? https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:679: else if (it->first == "num") Should "num" be qualified, just as "callNum" is? "instanceNum"? https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:695: } else...? It's undoubtedly a typo of some sort. https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:707: } else if (request_path == "nothing") { Is this clause needed by anything, or do you have something in the works for it?
Also, I didn't mention that I have an upcoming CL to allow the failure of LevelDBIterator's which will further complicate indexed_db_browsertest.cc. If you think that will make this file too busy then maybe these new classes (LevelDBTestTansaction, IndexedDBBrowserTestClassFactory, and the accompanying classes/enums) should go into a separate file(s) - thoughts? https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:215: test_class_factory_(new IndexedDBBrowserTestClassFactory) {} On 2014/06/17 01:12:53, ericu wrote: > Is test_class_factory_ actually used somewhere? If so, should it be a > scoped_ptr instead of raw? Embarrassingly no :-| https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:228: IndexedDBClassFactory::SetIndexedDBClassFactoryGetter(NULL); On 2014/06/17 01:12:53, ericu wrote: > Given this cleanup, do we need s_factory to be a static? I was able to eliminate the use of s_factory in CorruptDBRequestHandler by passing in a ptr to the test as a param. However, GetIDBClassFactory is the one function which still requires a static and I was unable to eliminate that requirement. I was able to reduce the scope of s_factory down to a single function, but now there are two factory getters (GetIDBClassFactory & GetTestClassFactory). Let me know if you see a better alternative. https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:679: else if (it->first == "num") On 2014/06/17 01:12:53, ericu wrote: > Should "num" be qualified, just as "callNum" is? "instanceNum"? How about "instNum"? https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:695: } On 2014/06/17 01:12:53, ericu wrote: > else...? It's undoubtedly a typo of some sort. Done. A DCHECK would have caught the error, but a VLOG is better. https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:707: } else if (request_path == "nothing") { On 2014/06/17 01:12:53, ericu wrote: > Is this clause needed by anything, or do you have something in the works for it? Oops - shouldn't have been there. I was experimenting with improving the test authoring workflow.
https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:679: else if (it->first == "num") On 2014/06/17 16:29:24, cmumford wrote: > On 2014/06/17 01:12:53, ericu wrote: > > Should "num" be qualified, just as "callNum" is? "instanceNum"? > > How about "instNum"? Sure. https://codereview.chromium.org/334303002/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:215: IndexedDBClassFactory::SetIndexedDBClassFactoryGetter(GetIDBClassFactory); Can you pass in GetTestClassFactory here, or is a function returning the derived type not close enough? https://codereview.chromium.org/334303002/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:704: VLOG(0) << "Unknown method: \"" << fail_method << '"'; Nit: It's strange to use two different methods to output a double-quote on a single line of output. How about just "\"" at the end, or use single-quote to surround fail_method in your output?
https://codereview.chromium.org/334303002/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:215: IndexedDBClassFactory::SetIndexedDBClassFactoryGetter(GetIDBClassFactory); On 2014/06/17 17:03:05, ericu wrote: > Can you pass in GetTestClassFactory here, or is a function returning the derived > type not close enough? Yes, I gave it a try, but compiler wasn't able to coerce one function pointer to another. I could force it, but it requires a reinterpret_cast which I try to avoid. https://codereview.chromium.org/334303002/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:704: VLOG(0) << "Unknown method: \"" << fail_method << '"'; On 2014/06/17 17:03:05, ericu wrote: > Nit: It's strange to use two different methods to output a double-quote on a > single line of output. How about just "\"" at the end, or use single-quote to > surround fail_method in your output? Done.
lgtm https://codereview.chromium.org/334303002/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:215: IndexedDBClassFactory::SetIndexedDBClassFactoryGetter(GetIDBClassFactory); On 2014/06/17 21:53:18, cmumford wrote: > On 2014/06/17 17:03:05, ericu wrote: > > Can you pass in GetTestClassFactory here, or is a function returning the > derived > > type not close enough? > > Yes, I gave it a try, but compiler wasn't able to coerce one function pointer to > another. I could force it, but it requires a reinterpret_cast which I try to > avoid. Yeah, let's stay away from that stuff. There be dragons.
This patch set just moves the new test classes (IndexedDBBrowserTestClassFactory, LevelDBTestTansaction, etc.) into their own file to keep indexed_db_browsertest.cc from getting too crazy. Please let me know if the prior implementation is preferred and I will revert this change. My worry is that an upcoming CL (which overrides LevelDBIterator) would just make indexed_db_browsertest.cc too busy.
I like it.
Just a few drive-by nits. https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:530: else { Nit: If any if/elseif/else branch has braces, all branches should have braces. https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:532: NOTREACHED(); Just `NOTREACHED() << message` ? (And then it can be a single line so no braces...) https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:542: else { Ditto. https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:544: NOTREACHED(); NOTREACHED() << ... ? https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest_mock_factory.cc (right): https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest_mock_factory.cc:37: } // anonymous namespace nit: Just `// namespace`
Forgot to indicate "Done" on Josh's comments. Addressed in patch #5. https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:532: NOTREACHED(); On 2014/06/18 17:03:08, jsbell wrote: > Just `NOTREACHED() << message` ? > > (And then it can be a single line so no braces...) Done. https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:544: NOTREACHED(); On 2014/06/18 17:03:08, jsbell wrote: > NOTREACHED() << ... ? Done. https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest_mock_factory.cc (right): https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest_mock_factory.cc:37: } // anonymous namespace On 2014/06/18 17:03:08, jsbell wrote: > nit: Just `// namespace` Done.
On 2014/06/18 18:06:44, cmumford wrote: > Forgot to indicate "Done" on Josh's comments. Addressed in patch #5. > > https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... > File content/browser/indexed_db/indexed_db_browsertest.cc (right): > > https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... > content/browser/indexed_db/indexed_db_browsertest.cc:532: NOTREACHED(); > On 2014/06/18 17:03:08, jsbell wrote: > > Just `NOTREACHED() << message` ? > > > > (And then it can be a single line so no braces...) > > Done. > > https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... > content/browser/indexed_db/indexed_db_browsertest.cc:544: NOTREACHED(); > On 2014/06/18 17:03:08, jsbell wrote: > > NOTREACHED() << ... ? > > Done. > > https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... > File content/browser/indexed_db/indexed_db_browsertest_mock_factory.cc (right): > > https://codereview.chromium.org/334303002/diff/60001/content/browser/indexed_... > content/browser/indexed_db/indexed_db_browsertest_mock_factory.cc:37: } // > anonymous namespace > On 2014/06/18 17:03:08, jsbell wrote: > > nit: Just `// namespace` > > Done. Josh: Any other comments on this CL?
Sorry, didn't know you were waiting for me. lgtm with various nits... (I think the query-string parsing is fine as is, you can ignore those notes.) https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:486: // Remove the query string if present. Wasn't there a thread about a utility for query string parsing? https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:518: for (std::vector<std::pair<std::string, std::string> >::iterator it = Might be marginally more readable if the vector<> above was just iterated over to produce a map<> and then this could just pluck the values out. But this is fine. https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:545: DCHECK(instance_num >= 1); DCHECK_GE https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:546: DCHECK(call_num >= 1); DCHECK_GE https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest_mock_factory.cc (right): https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest_mock_factory.cc:126: if (failure_class_ == FAIL_CLASS_LEVELDB_TRANSACTION && Need braces for this if block (and therefore for the else block too) https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest_mock_factory.h (right): https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest_mock_factory.h:28: class IndexedDBBrowserTestClassFactory : public IndexedDBClassFactory { Looking around the Chromium code, it's more common to name these MockXXX e.g. MockIndexedDBClassFactory, with the file named mock_xxx.h Does this need the BrowserTest qualifier? https://codereview.chromium.org/334303002/diff/80001/content/test/data/indexe... File content/test/data/indexeddb/corrupted_open_db_detection.html (right): https://codereview.chromium.org/334303002/diff/80001/content/test/data/indexe... content/test/data/indexeddb/corrupted_open_db_detection.html:113: testXhr("/corrupt/test/fail?class=LevelDBTransaction&method=Get&instNum=1", function() { Wrap this so it's less than 80 chars?
https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:545: DCHECK(instance_num >= 1); On 2014/06/19 21:08:47, jsbell wrote: > DCHECK_GE Done. https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:546: DCHECK(call_num >= 1); On 2014/06/19 21:08:47, jsbell wrote: > DCHECK_GE Done. https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest_mock_factory.cc (right): https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest_mock_factory.cc:126: if (failure_class_ == FAIL_CLASS_LEVELDB_TRANSACTION && On 2014/06/19 21:08:47, jsbell wrote: > Need braces for this if block (and therefore for the else block too) Done. https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest_mock_factory.h (right): https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest_mock_factory.h:28: class IndexedDBBrowserTestClassFactory : public IndexedDBClassFactory { On 2014/06/19 21:08:47, jsbell wrote: > Looking around the Chromium code, it's more common to name these MockXXX e.g. > MockIndexedDBClassFactory, with the file named mock_xxx.h > > Does this need the BrowserTest qualifier? I'll rename to MockXXX. Here's where I was going with the names: I've been thinking that Mock<ClassName> named classes would be "pure mock" objects (See https://codereview.chromium.org/308103004/diff/60001/content/browser/indexed_... for example). Most tests just need a phony mock object so that they can instantiate the actual class they want to test. This is the most reusable type of mock object. A test that wants more specific behavior would create their own mock implementation (ideally in that tests's *.cc file) - and then it would be named something like Mock<TestName><ClassName>. I can also see mock objects that serve a specific purpose, but can be shared between tests - and I think this might be a candidate. Then it would be named Mock<WhatItDoes><ClassName>. I can't think of a good "WhatItDoes" name for this - "MockFailingIndexedDBClassFactory" just doesn't sound right. So I don't yet have any other tests that I can see using what is currently named IndexedDBBrowserTestClassFactory. What do you think about naming it MockBrowserTestIndexedDBClassFactory, and then renaming it to something else if/when it needs to be reused? I'm not married to any of this so let me know what you prefer.
https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest_mock_factory.h (right): https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest_mock_factory.h:28: class IndexedDBBrowserTestClassFactory : public IndexedDBClassFactory { On 2014/06/19 22:17:11, cmumford wrote: > On 2014/06/19 21:08:47, jsbell wrote: > > Looking around the Chromium code, it's more common to name these MockXXX e.g. > > MockIndexedDBClassFactory, with the file named mock_xxx.h > > > > Does this need the BrowserTest qualifier? > > I'll rename to MockXXX. > > Here's where I was going with the names: I've been thinking that Mock<ClassName> > named classes would be "pure mock" objects (See > https://codereview.chromium.org/308103004/diff/60001/content/browser/indexed_... > for example). Most tests just need a phony mock object so that they can > instantiate the actual class they want to test. This is the most reusable type > of mock object. > > A test that wants more specific behavior would create their own mock > implementation (ideally in that tests's *.cc file) - and then it would be named > something like Mock<TestName><ClassName>. > > I can also see mock objects that serve a specific purpose, but can be shared > between tests - and I think this might be a candidate. Then it would be named > Mock<WhatItDoes><ClassName>. I can't think of a good "WhatItDoes" name for this > - "MockFailingIndexedDBClassFactory" just doesn't sound right. > > So I don't yet have any other tests that I can see using what is currently named > IndexedDBBrowserTestClassFactory. What do you think about naming it > MockBrowserTestIndexedDBClassFactory, and then renaming it to something else > if/when it needs to be reused? > > I'm not married to any of this so let me know what you prefer. I'm good with MockBrowserTestIndexedDBClassFactory for now.
https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/334303002/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_browsertest.cc:518: for (std::vector<std::pair<std::string, std::string> >::iterator it = On 2014/06/19 21:08:47, jsbell wrote: > Might be marginally more readable if the vector<> above was just iterated over > to produce a map<> and then this could just pluck the values out. But this is > fine. I switched to url::ExtractQueryKeyValue, and although slightly more verbose I think it reads easier and is more correct.
The CQ bit was checked by cmumford@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cmumford@chromium.org/334303002/100001
The CQ bit was checked by cmumford@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cmumford@chromium.org/334303002/120001
Message was sent while issue was closed.
Change committed as 278838 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
