1315138 - gtestify storage/test/*.cpp

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect, P3)

Description

[Nicholas Nethercote [inactive]]

We are getting rid of GeckoCppUnitTests for bug 1313141. This bug is about converting the ones in storage/test/*.cpp to gtest.

Comment 1

[Nicholas Nethercote [inactive]]

This depends on bug 1313488 because storage/test/test_deadlock_detector.cpp is a slightly modified version of xpcom/test/TestDeadlockDetector.cpp.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: gtestify storage/test/*.cpp

erahm, please review the test_deadlock_detector.cpp/TestDeadlockDetector.cpp
changes.

mak, please review all the storage/ changes.

Thank you.

Comment 3

[Eric Rahm [:erahm]]

Comment on attachment 8809651 [details] [diff] [review]
gtestify storage/test/*.cpp

Review of attachment 8809651 [details] [diff] [review]:
-----------------------------------------------------------------

I don't love this, but I guess it's better than copying the file.

Comment 4

[Nicholas Nethercote [inactive]]

> I don't love this, but I guess it's better than copying the file.

Agreed!

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8809651 [details] [diff] [review]
gtestify storage/test/*.cpp

Review of attachment 8809651 [details] [diff] [review]:
-----------------------------------------------------------------

::: storage/test/gtest/storage_test_harness.h
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef storage_test_harness_h__
> +#define storage_test_harness_h__

nit: I think all of our compilers now support #pragma once

::: storage/test/gtest/test_StatementCache.cpp
@@ +54,5 @@
> +// This is some gtest magic that allows us to parameterize tests by |const
> +// char[]| and |StringWrapper|.
> +template <typename T>
> +class StatementCacheTest : public ::testing::Test {};
> +typedef ::testing::Types<const char[], StringWrapper> TwoStrings;

nit: TwoStringTypes?

::: storage/test/gtest/test_unlock_notify.cpp
@@ +158,5 @@
> +    do_check_success(rv);
> +  }
> +
> +  // test_step_locked_does_not_block_main_thread
> +  {

What's the reason we can't keep these tests in separate functions? The test harness doesn't guarantee order?
If we need to do this, I'd like at least to have some dump at the beginning of a sub test. But I still prefer separated functions if it's possible.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to Marco Bonardo [::mak] from comment #5)
> Comment on attachment 8809651 [details] [diff] [review]
> gtestify storage/test/*.cpp
> 
> Review of attachment 8809651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: storage/test/gtest/storage_test_harness.h
> @@ +4,5 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +#ifndef storage_test_harness_h__
> > +#define storage_test_harness_h__
> 
> nit: I think all of our compilers now support #pragma once

glandium tells me it's not safe for us to use #pragma once because we copy headers to dist/include, and with #pragma once it might end up being included twice. https://groups.google.com/forum/#!msg/mozilla.dev.platform/PgDjWw3xp8k/PLYQc5xoWmsJ has more details.

> nit: TwoStringTypes?

Ok.

> What's the reason we can't keep these tests in separate functions? The test
> harness doesn't guarantee order?
> If we need to do this, I'd like at least to have some dump at the beginning
> of a sub test. But I still prefer separated functions if it's possible.

I can put them back into separate functions. But they have to be in a single TEST because each TEST can be executed by itself.

Comment 7

[Nicholas Nethercote [inactive]]

There are two problems that show up when I run all the storage gtests together.

- test_service_init_background_thread fails. Specifically, the do_check_false()
  call within Run() fails because do_GetService() unexpectedly succeeds. This
  only happens when test_StatementCache runs beforehand.

- test_true_async hangs, because the getMemoryDatabase() call inside it hangs 
  due to some kind of deadlock within SQLite. This only happens when
  test_transaction_helper() runs beforehand. I traced through in GDB and got
  the following call chain:

  getMemoryDatabase() -> 
  Service::OpenSpecialDatabase() -> 
  Connection::initialize() -> 
  sqlite3_open_v2() -> 
  openDatabase() ->
  sqlite3_initialize() ->
  sqlite3_mutex_enter(), which hangs. (I didn't trace any deeper than that.)

mak, any ideas on how to fix these two problems? Thanks.

Comment 8

[Nicholas Nethercote [inactive]]

> - test_service_init_background_thread fails. Specifically, the
> do_check_false()
>   call within Run() fails because do_GetService() unexpectedly succeeds. This
>   only happens when test_StatementCache runs beforehand.

Also, the comment at the top of this file says:

> * This file tests that the storage service can be initialized off of the main
> * thread without issue.

But AFAICT it is actually testing that off-main-thread initialization fails. Is the comment wrong?

Comment 9

[Marco Bonardo [:mak]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> > - test_service_init_background_thread fails. Specifically, the
> > do_check_false()
> >   call within Run() fails because do_GetService() unexpectedly succeeds. This
> >   only happens when test_StatementCache runs beforehand.
> 
> Also, the comment at the top of this file says:
> 
> > * This file tests that the storage service can be initialized off of the main
> > * thread without issue.
> 
> But AFAICT it is actually testing that off-main-thread initialization fails.
> Is the comment wrong?

the original commit was right:

https://hg.mozilla.org/mozilla-central/rev/f9e77a6f0adc

Then in bug 836493 we changed so that the service is always initialized on the main-thread. The test code was updated but the comments were not.

So the comments need to be fixed.

I suspect your problem may be that this uses do_GetService, if a previous test already initialized the service, a further call to do_GetService will just increase the ref counter, and that wouldn't activate our assertion code.
I wonder if we could change this to a do_CreateInstance?

I'm still looking into the other question.

Comment 10

[Marco Bonardo [:mak]]

The other problem may be due to this:
http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/storage/test/test_transaction_helper.cpp#123

I suspect this test cannot live along with the others, as-is, it's doing low-level hacks, and should probably be the latest test we run.
How are gTests packed, is it one executable per folder, or one for all the existing tests around?
Alternatively, you could try to implement an unhook_sqlite_mutex that tries to restore things as they were originally.

But first, could you please check if commenting out test_async_Commit solves the problem?

Comment 11

[Benjamin Smedberg]

We could probably mark this as a DeathTest which will run it independently of the others.

Comment 12

[Nicholas Nethercote [inactive]]

> I suspect your problem may be that this uses do_GetService, if a previous
> test already initialized the service, a further call to do_GetService will
> just increase the ref counter, and that wouldn't activate our assertion code.
> I wonder if we could change this to a do_CreateInstance?

I tried changing the do_GetService to do_CreateInstance and it fails in the same way.

Could we just remove this test? The original purpose of the test -- make sure the service can be created off the main thread -- makes sense to me. The updated purpose seems less important.

Comment 13

[Nicholas Nethercote [inactive]]

> I suspect this test cannot live along with the others, as-is, it's doing
> low-level hacks, and should probably be the latest test we run.
> How are gTests packed, is it one executable per folder, or one for all the
> existing tests around?

AIUI all the gtests, from every directory, get put into a single executable. Though you can create death tests which run in a separate process, as bsmedberg mentioned.

> Alternatively, you could try to implement an unhook_sqlite_mutex that tries
> to restore things as they were originally.
> 
> But first, could you please check if commenting out test_async_Commit solves
> the problem?

It does fix the hang, yes.

Comment 14

[Marco Bonardo [:mak]]

(In reply to Nicholas Nethercote [:njn] from comment #12)
> Could we just remove this test? The original purpose of the test -- make
> sure the service can be created off the main thread -- makes sense to me.
> The updated purpose seems less important.

On the contrary, it's more important to check that we fail to create the service off the main-thread, cause we can't guarantee thread-safety and thus we should never allow a consumer to do that.
Maybe we could also run this test separately as suggested by Benjamin?

Comment 15

[Nicholas Nethercote [inactive]]

I tried doing a death test and hit all sorts of weird problems. Here's one of several try runs I did:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f2e297189df6035efd93f8817006748e7dfa6fd

Results:

- Windows: all green.

- Mac: assertion failures in the JS engine, something to do with threading.

- Linux: a whole mix of things. Some greens, some assertion failures in NSS (something involving NSS being unhappy when it detects it has been forked), some crashes. I tried the default "fast" death test style and also the supposedly safer "threadsafe" style and both gave similar results.

Death tests are kind of dodgy in the presence of threads, which we have lots of... at this point I'm going to try to avoid them.


> Alternatively, you could try to implement an unhook_sqlite_mutex that tries to restore things as they were originally.

I've done this now and it seems to have fixed the hang. Now I just have to fix the off-thread initialization success; I might just do that by restructuring the code to ensure it runs before the other code.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: gtestify storage/test/*.cpp

I found solutions to the two problems, one good, and one hacky.

Comment 17

[Andi [:andi]]

As per this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1318282
We should have this modified:

::: storage/test/test_AsXXX_helpers.cpp:24
(Diff revision 1)
>  public:
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_ASYNCSTATEMENTSPINNER
>    Spinner() {}
>  protected:
> -  virtual ~Spinner() = default;
> +  ~Spinner() override = default;

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8813046 [details] [diff] [review]
gtestify storage/test/*.cpp

Review of attachment 8813046 [details] [diff] [review]:
-----------------------------------------------------------------

It looks acceptable, I wonder if the death test hack may create problems in future, but if it's the only way to force the test to run as the first one... I took a quick look at the docs, and doesn't look like there's an easy way to enforce an order.

Please also fix comment 17.

Comment 19

[Nicholas Nethercote [inactive]]

> It looks acceptable, I wonder if the death test hack may create problems in
> future, but if it's the only way to force the test to run as the first
> one... I took a quick look at the docs, and doesn't look like there's an
> easy way to enforce an order.

Yeah, it's not great, but the only way I can see it causing problems in the future is if someone else creates a similar test -- with a "DeathTest" suffix, but not actually a real death test that runs in another process -- that also initializes the storage service. Which seems unlikely. If it happens, that new test could be combined with the existing test.

> Please also fix comment 17.

Will do.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2bbd06a80c
gtestify storage/test/*.cpp. r=mak,erahm.

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7f2bbd06a80c