Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784003002:
[IndexedDB] Mojo testing harness. (Closed)
|
Created:
3 years, 8 months ago by dmurph Modified:
3 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, cmumford, darin (slow to review), darin-cc_chromium.org, jam, jsbell+idb_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[IndexedDB] Mojo testing harness & tests.
This creates a testing harness for the IndexedDB system from the mojo
layer.
Included tests:
* Opening connection and immediately closing.
* Opening connection, upgrading database, and closing.
* Opening two connection, upgrading on one.
* Calling 'put' with an invalid blob.
There is some minimal state storage for each connection, and we have
mock objects for the callbacks.
Review-Url: https://codereview.chromium.org/2784003002
Cr-Commit-Position: refs/heads/master@{#472563}
Committed: https://chromium.googlesource.com/chromium/src/+/a4c5acae0c20cb76eb34f2703ab14b8ca6d0a6cf
Patch Set 1 #Patch Set 2 : Working! #
Total comments: 13
Patch Set 3 : comments #
Total comments: 1
Patch Set 4 : Restructured #
Total comments: 4
Patch Set 5 : comments #
Total comments: 10
Patch Set 6 : comments, removed thread flushing #
Total comments: 1
Messages
Total messages: 44 (22 generated)
Description was changed from ========== [IndexedDB] Mojo testing harness. ========== to ========== [IndexedDB] Mojo testing harness & tests. This creates a testing harness for the IndexedDB system from the mojo layer. Included tests: * Opening connection and immediately closing. * Opening connection, upgrading database, and closing. * ... TODO: Create a way to not execute delayed tasks on TestSimpleTaskRunner ==========
dmurph@chromium.org changed reviewers: + jam@chromium.org, reillyg@chromium.org
Hello John & Reilly, Can you PTAL at this WIP patch? Please look especially at the mock_mojo classes and the indexed_db_dispatcher_host_unittest.cc class. There's quite a bit of boilerplate there, and while this is working, I'm a little scared of all the raw pointers I need to keep around (and confused why I have explicitly call Close on strong bindings). Are there things I can do here to simplify? Are there things I can change in mojo to simplify? Thanks, Daniel
dmurph@chromium.org changed reviewers: + rockot@chromium.org
+Rockot, same request as above, can you PTAL at this patch? It's a WIP (still want to add more test harness infrastructure and tests) but the mojo layer is the best I can get it. Earlier I was asking about mojo testing infrastructure, this is where was heading.
https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_callbacks.h (right): https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_callbacks.h:149: std::unique_ptr<IOThreadHelper, DeleteOnIOThreadTaskRunner> io_helper_; Why did you copy this code from BrowserThread::DeleteOnIOThread?
https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:92: DatabaseAssociatedPtr* database; It's hard to tell what is going on in the methods on these structs because the member variable names don't end in an underscore. https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:143: version, upgrade_txn_id); First, I suggest adding a method to mojo::AssociatedBinding called CreateInterfacePtrInfoAndBind which does the same thing as mojo::Binding::CreateInterfacePtrAndBind but works for associated interfaces. Then, instead of using mojo::MakeStrongAssociatedBinding here I would add a mojo::AssociatedBinding member to MockMojoIndexedDBCallbacks and add a method which calls your new CreateInterfacePtrAndBind method to create the ptr_info arguments to this method. Then TestDatabaseConnection can own unique_ptrs to the mock callback objects instead of having the raw pointers you find awkward. The code would then be: open_callbacks = base::MakeUnique<testing::StrictMock<MockMojoIndexedDBCallbacks>>(); connection_callbacks = base::MakeUnique<testing::StrictMock<MockMojoIndexedDBDatabaseCallbacks>>(); (*factory)->Open(open_callbacks->CreateInterfacePtrInfoAndBind(), connection_callbacks->CreateInterfacePtrInfoAndBind(), origin, db_name, version, upgrade_txn_id);
On Wed, May 10, 2017 at 7:12 PM, <reillyg@chromium.org> wrote: > > https://codereview.chromium.org/2784003002/diff/20001/ > content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc > File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc > (right): > > https://codereview.chromium.org/2784003002/diff/20001/ > content/browser/indexed_db/indexed_db_dispatcher_host_ > unittest.cc#newcode92 > content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:92: > DatabaseAssociatedPtr* database; > It's hard to tell what is going on in the methods on these structs > because the member variable names don't end in an underscore. > > https://codereview.chromium.org/2784003002/diff/20001/ > content/browser/indexed_db/indexed_db_dispatcher_host_ > unittest.cc#newcode143 > content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:143: > version, upgrade_txn_id); > First, I suggest adding a method to mojo::AssociatedBinding called > CreateInterfacePtrInfoAndBind which does the same thing as > mojo::Binding::CreateInterfacePtrAndBind but works for associated > interfaces. > I would actually recommend against this. I was chatting with jam@ the other day about the problem of having too many ways to do the same thing. We concluded that we should start *removing* methods like "CreateInterfacePtrAndBind", and standardize everyone on MakeRequest and MakeProxy (which doesn't quite yet exist in the right form.) > Then, instead of using mojo::MakeStrongAssociatedBinding here I would > add a mojo::AssociatedBinding member to MockMojoIndexedDBCallbacks and > add a method which calls your new CreateInterfacePtrAndBind method to > create the ptr_info arguments to this method. > > Then TestDatabaseConnection can own unique_ptrs to the mock callback > objects instead of having the raw pointers you find awkward. > > The code would then be: > > open_callbacks = > base::MakeUnique<testing::StrictMock<MockMojoIndexedDBCallbacks>>(); > connection_callbacks = > base::MakeUnique<testing::StrictMock<MockMojoIndexedDBDatabaseCallb > acks>>(); > > (*factory)->Open(open_callbacks->CreateInterfacePtrInfoAndBind(), > connection_callbacks->CreateInterfacePtrInfoAndBind(), > origin, db_name, version, upgrade_txn_id); > > https://codereview.chromium.org/2784003002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Seems mostly reasonable to me. The whole gmock-not-supporting-move-only-parameters thing is kind of sad. Are the facilities of gmock valuable enough that it's still worth using here, or could we get away with a less mock-y testing strategy? e.g. one (strawman) idea might be to have a base testing impl of these callbacks interfaces, and then provide some facility for individual tests to override a subset of the API for each test's isolated needs. https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:92: DatabaseAssociatedPtr* database; This could be a raw mojom::Database*. No need to hold a raw * to the InterfacePtr itself unless you're doing something other than dereferencing it for interface calls. https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:143: version, upgrade_txn_id); On 2017/05/11 at 02:12:01, Reilly Grant wrote: > First, I suggest adding a method to mojo::AssociatedBinding called CreateInterfacePtrInfoAndBind which does the same thing as mojo::Binding::CreateInterfacePtrAndBind but works for associated interfaces. > > Then, instead of using mojo::MakeStrongAssociatedBinding here I would add a mojo::AssociatedBinding member to MockMojoIndexedDBCallbacks and add a method which calls your new CreateInterfacePtrAndBind method to create the ptr_info arguments to this method. > > Then TestDatabaseConnection can own unique_ptrs to the mock callback objects instead of having the raw pointers you find awkward. > > The code would then be: > > open_callbacks = base::MakeUnique<testing::StrictMock<MockMojoIndexedDBCallbacks>>(); > connection_callbacks = base::MakeUnique<testing::StrictMock<MockMojoIndexedDBDatabaseCallbacks>>(); > > (*factory)->Open(open_callbacks->CreateInterfacePtrInfoAndBind(), > connection_callbacks->CreateInterfacePtrInfoAndBind(), > origin, db_name, version, upgrade_txn_id); Guess to be safe my reply should have been inline, so just to reiterate what I replied in email, please don't add another CreateInterfacePtrAndBind method. https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:194: StrongAssociatedBindingPtr<Callbacks> open_callbacks_strong_ptr; If you actually have to explicitly close these strong bindings on teardown it must be because of some expectations the mock classes have. The only side effect of *not* closing these bindings is that the bound mocks may still receive message calls after the test fixture is destroyed (which is indeed weird behavior and you should almost certainly avoid it.) I would recommend doing what Reilly suggests, with a slight variation. Instead of using CreateInterfacePtrAndBind, have the constructors take a corresponding AssociatedInterfaceRequest (e.g. CallbacksAssociatedRequest), and then your code still simplifies to: mojom::CallbacksAssociatedPtrInfo callbacks_ptr_info; mojom::DatabaseCallbacksAssociatedPtrInfo database_callbacks_ptr_info; open_callbacks_ = base::MakeUnique<testing::StrictMock<MockMojoIndexedDBCallbacks>>( mojo::MakeRequest(&callbacks_ptr_info)); connection_callbacks_ = base::MakeUnique<testing::StrictMock<MockMojoIndexedDBDatabaseCallbacks>>( mojo::MakeRequest(&database_callbacks_ptr_info)); https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:224: base::RunLoop().RunUntilIdle(); Please never use RunUntilIdle unless you are explicitly testing MessageLoop behavior and have fine control over all tasks running. This usage effectively relies on tons of subtle implementation details which are subject to change and will almost inevitably make your tests hard to maintain. If you need to wait for a specific event to occur, the proper thing to do is plumb a RunLoop's QuitClosure() to the right place and have that event run the closure; then you simply Run() the loop. https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:229: base::RunLoop().RunUntilIdle(); Again with the RunUntilIdle() - flakiness danger zone! (apologies if these are just temporary for the purpose of hacking)
Thanks so much for the comments! What do you think now? It's cleaned up a lot - I'm probably going to split the harness out, especially because I need a somewhat complex quota manager setup to get that to work. https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_callbacks.h (right): https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_callbacks.h:149: std::unique_ptr<IOThreadHelper, DeleteOnIOThreadTaskRunner> io_helper_; On 2017/05/11 01:57:08, Reilly Grant wrote: > Why did you copy this code from BrowserThread::DeleteOnIOThread? Not a perfect copy - DeleteOnIOThread actually deletes the object immediately if we're on the correct thread. Because the code relies on this around scheduling tasks inside the class, and in the unittests I can't change the 'thread' I'm on, I have to do this. Let me know if you know a better way! https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:194: StrongAssociatedBindingPtr<Callbacks> open_callbacks_strong_ptr; On 2017/05/11 17:21:03, Ken Rockot wrote: > If you actually have to explicitly close these strong bindings on teardown it > must be because of some expectations the mock classes have. The only side effect > of *not* closing these bindings is that the bound mocks may still receive > message calls after the test fixture is destroyed (which is indeed weird > behavior and you should almost certainly avoid it.) > > I would recommend doing what Reilly suggests, with a slight variation. Instead > of using CreateInterfacePtrAndBind, have the constructors take a corresponding > AssociatedInterfaceRequest (e.g. CallbacksAssociatedRequest), and then your code > still simplifies to: > > mojom::CallbacksAssociatedPtrInfo callbacks_ptr_info; > mojom::DatabaseCallbacksAssociatedPtrInfo database_callbacks_ptr_info; > open_callbacks_ = > base::MakeUnique<testing::StrictMock<MockMojoIndexedDBCallbacks>>( > mojo::MakeRequest(&callbacks_ptr_info)); > connection_callbacks_ = > base::MakeUnique<testing::StrictMock<MockMojoIndexedDBDatabaseCallbacks>>( > mojo::MakeRequest(&database_callbacks_ptr_info)); Thanks! https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:224: base::RunLoop().RunUntilIdle(); On 2017/05/11 17:21:03, Ken Rockot wrote: > Please never use RunUntilIdle unless you are explicitly testing MessageLoop > behavior and have fine control over all tasks running. This usage effectively > relies on tons of subtle implementation details which are subject to change and > will almost inevitably make your tests hard to maintain. If you need to wait for > a specific event to occur, the proper thing to do is plumb a RunLoop's > QuitClosure() to the right place and have that event run the closure; then you > simply Run() the loop. I need to do this because I need to run the IO thread from the IDB perspective (it schedules tasks on the io thread to reply back). What else should I use? I actually need this one to get the blob storage context initialized, as it schedules an IO thread task to initialize. https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:229: base::RunLoop().RunUntilIdle(); On 2017/05/11 17:21:03, Ken Rockot wrote: > Again with the RunUntilIdle() - flakiness danger zone! (apologies if these are > just temporary for the purpose of hacking) These are temp, above is not though.
The CQ bit was checked by dmurph@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...
https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_callbacks.h (right): https://codereview.chromium.org/2784003002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_callbacks.h:149: std::unique_ptr<IOThreadHelper, DeleteOnIOThreadTaskRunner> io_helper_; On 2017/05/12 01:22:06, dmurph wrote: > > On 2017/05/11 01:57:08, Reilly Grant wrote: > > Why did you copy this code from BrowserThread::DeleteOnIOThread? > > Not a perfect copy - DeleteOnIOThread actually deletes the object immediately if > we're on the correct thread. Because the code relies on this around scheduling > tasks inside the class, and in the unittests I can't change the 'thread' I'm on, > I have to do this. I don't understand this last sentence. In unit tests you can still post tasks to other threads. If you need to perform an action on the IDB thread and make sure that tasks it posts to the IO thread happen before the run loop finishes you can do this, base::RunLoop loop; db_task_runner_->PostTaskAndReply(FROM_HERE, idb_task_closure, loop.QuitClosure()); loop.Run(); https://codereview.chromium.org/2784003002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2784003002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:134: class TestDatabaseConnection { I recommend against using a class like this because it makes it less clear what is going on in each of the test functions. Once you have a number of test cases we can see if there are common pieces that we will want to factor out.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dmurph@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...
dmurph@chromium.org changed reviewers: + pwnall@chromium.org
Thanks for the comments! Please take another look - I'd actually like to submit something close to this for an initial version. Reilly helped me out a lot with the restructuring, and I think it's looking pretty good. There's now a specific thread for IDB. I ran into issues trying to join it, specifically here: https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_co... Where that will actually fail to use the IDB thread - if there is no longer an IDB thread haha. So it destructs on the IO thread. So I'll try to figure this out. I'm a little worries about possible deadlocks or freezes. I wonder if I could have gmock execute quit closures when there are errors. +Victor for FYI
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
(test failures from test dcheck I put in there- forgot to remove)
https://codereview.chromium.org/2784003002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2784003002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:426: new StrictMock<MockMojoIndexedDBCallbacks>()); auto put_callbacks = base::MakeUnique<StrictMock<MockMojoIndexedDBCallbacks>>(); https://codereview.chromium.org/2784003002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:459: connection.database.FlushForTesting(); Why is this necessary? |quit_closure| should be handling making sure that execution continues until the all the expected callbacks happen. If you want to assert that no extra callbacks happen I would put that after loop.Run().
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Description was changed from ========== [IndexedDB] Mojo testing harness & tests. This creates a testing harness for the IndexedDB system from the mojo layer. Included tests: * Opening connection and immediately closing. * Opening connection, upgrading database, and closing. * ... TODO: Create a way to not execute delayed tasks on TestSimpleTaskRunner ========== to ========== [IndexedDB] Mojo testing harness & tests. This creates a testing harness for the IndexedDB system from the mojo layer. Included tests: * Opening connection and immediately closing. * Opening connection, upgrading database, and closing. * Opening two connection, upgrading on one. * Calling 'put' with an invalid blob. There is some minimal state storage for each connection, and we have mock objects for the callbacks. ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ken & Reilly: can you PTAL? John: This patch is an FYI about what is needed to test a mojo service at the mojo level. https://codereview.chromium.org/2784003002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2784003002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:426: new StrictMock<MockMojoIndexedDBCallbacks>()); On 2017/05/15 21:24:33, Reilly Grant wrote: > auto put_callbacks = base::MakeUnique<StrictMock<MockMojoIndexedDBCallbacks>>(); Done. https://codereview.chromium.org/2784003002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:459: connection.database.FlushForTesting(); On 2017/05/15 21:24:33, Reilly Grant wrote: > Why is this necessary? |quit_closure| should be handling making sure that > execution continues until the all the expected callbacks happen. If you want to > assert that no extra callbacks happen I would put that after loop.Run(). Done.
https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:288: // extra comment? https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:347: FlushIORunLoop(); I'm not convinced all this flushing is necessary.
The mojom stuff all looks great, but I think the waiting-for-stuff-to-happen behavior definitely needs to change before this can land. https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:162: FlushIORunLoop(); This is not really a better testing strategy than RunUntilIdle, and it it suffers from the same basic problem of making dubious assumptions about the internal behavior of the system. Suppose that one of the actions triggered above ends up changing to take place over multiple task scheduler hops. Now your "flush" operation is not sufficient to ensure completion. Do you really want your test coverage to enforce the constraint that such operations must always complete within one pump of the message loop? If you want to wait for the quotas to be set, you should plumb some reasonable (possibly test-only) API into the quota manager that you can observe and wait for explicitly. This comment applies generally to all uses of FlushIORunLoop and FlushIDBThreadRunLoop in these tests. You should wait for explicit events, not for arbitrary message loop iterations. https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:185: FlushIORunLoop(); This sequence of "flush" operations is a gigantic red flag, re the above comment https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:476: LOG(ERROR) << "timout"; nit: remove
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Thanks for the comments! PTAL - I removed all of the thread flushing and join the threads explicitly after de-refing the refpointers (with a comment explaining why). https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:162: FlushIORunLoop(); On 2017/05/16 21:06:32, Ken Rockot wrote: > This is not really a better testing strategy than RunUntilIdle, and it it > suffers from the same basic problem of making dubious assumptions about the > internal behavior of the system. > > Suppose that one of the actions triggered above ends up changing to take place > over multiple task scheduler hops. Now your "flush" operation is not sufficient > to ensure completion. Do you really want your test coverage to enforce the > constraint that such operations must always complete within one pump of the > message loop? > > If you want to wait for the quotas to be set, you should plumb some reasonable > (possibly test-only) API into the quota manager that you can observe and wait > for explicitly. > > This comment applies generally to all uses of FlushIORunLoop and > FlushIDBThreadRunLoop in these tests. You should wait for explicit events, not > for arbitrary message loop iterations. My main concern here is shutdown. If we have hops between idb and io threads on shutdown, I'm not sure how to get these to all execute and still have the task runners valid. I can join the threads, but that means that the idb task runner is essentially a no-op. I THINK this should be ok. See next patch, and if all tests pass. https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:185: FlushIORunLoop(); On 2017/05/16 21:06:32, Ken Rockot wrote: > This sequence of "flush" operations is a gigantic red flag, re the above comment I re-did this to deref all objects before joining threads. Let me know how this looks. https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:288: // On 2017/05/16 20:55:13, Reilly Grant wrote: > extra comment? Done. https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:347: FlushIORunLoop(); On 2017/05/16 20:55:13, Reilly Grant wrote: > I'm not convinced all this flushing is necessary. You're right - the InSequence constraint should handle this. https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2784003002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:476: LOG(ERROR) << "timout"; On 2017/05/16 21:06:32, Ken Rockot wrote: > nit: remove Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2784003002/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2784003002/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:164: void TearDown() override { Thanks, this seems better! One more change here though. If the same TestBrowserBrowserThreadBundle is used across all tests, you may leak tasks across test boundaries and that can have weird side effects or cause surprising/confusing test output. I would recommend constructing TestBrowserThreadBundle in SetUp and destroying it in TearDown (after you stop the IDB thread, I suppose)
On 2017/05/16 23:12:47, Ken Rockot wrote: > https://codereview.chromium.org/2784003002/diff/100001/content/browser/indexe... > File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): > > https://codereview.chromium.org/2784003002/diff/100001/content/browser/indexe... > content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:164: void > TearDown() override { > Thanks, this seems better! > > One more change here though. If the same TestBrowserBrowserThreadBundle is used > across all tests, you may leak tasks across test boundaries and that can have > weird side effects or cause surprising/confusing test output. I would recommend > constructing TestBrowserThreadBundle in SetUp and destroying it in TearDown > (after you stop the IDB thread, I suppose) It should be destructed/constructed for every test w/ the actual test object, right? We construct it in our constructor when we create the world, and destroy it when the test object is destroyed. They object doesn't live between tests.
On 2017/05/16 at 23:21:59, dmurph wrote: > On 2017/05/16 23:12:47, Ken Rockot wrote: > > https://codereview.chromium.org/2784003002/diff/100001/content/browser/indexe... > > File content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc (right): > > > > https://codereview.chromium.org/2784003002/diff/100001/content/browser/indexe... > > content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc:164: void > > TearDown() override { > > Thanks, this seems better! > > > > One more change here though. If the same TestBrowserBrowserThreadBundle is used > > across all tests, you may leak tasks across test boundaries and that can have > > weird side effects or cause surprising/confusing test output. I would recommend > > constructing TestBrowserThreadBundle in SetUp and destroying it in TearDown > > (after you stop the IDB thread, I suppose) > > It should be destructed/constructed for every test w/ the actual test object, right? We construct it in our constructor when we create the world, and destroy it when the test object is destroyed. They object doesn't live between tests. Sorry, duh, you're right. :) LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Reilly, can you PTAL?
lgtm
The CQ bit was checked by dmurph@chromium.org
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": 100001, "attempt_start_ts": 1495039414917700,
"parent_rev": "4e3c5b5568c8a4f19d42f8acaa6be76f620f1595", "commit_rev":
"a4c5acae0c20cb76eb34f2703ab14b8ca6d0a6cf"}
Message was sent while issue was closed.
Description was changed from ========== [IndexedDB] Mojo testing harness & tests. This creates a testing harness for the IndexedDB system from the mojo layer. Included tests: * Opening connection and immediately closing. * Opening connection, upgrading database, and closing. * Opening two connection, upgrading on one. * Calling 'put' with an invalid blob. There is some minimal state storage for each connection, and we have mock objects for the callbacks. ========== to ========== [IndexedDB] Mojo testing harness & tests. This creates a testing harness for the IndexedDB system from the mojo layer. Included tests: * Opening connection and immediately closing. * Opening connection, upgrading database, and closing. * Opening two connection, upgrading on one. * Calling 'put' with an invalid blob. There is some minimal state storage for each connection, and we have mock objects for the callbacks. Review-Url: https://codereview.chromium.org/2784003002 Cr-Commit-Position: refs/heads/master@{#472563} Committed: https://chromium.googlesource.com/chromium/src/+/a4c5acae0c20cb76eb34f2703ab1... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/a4c5acae0c20cb76eb34f2703ab1... |
