830884 - Fennec ContentProviders should call notifyChange in bulkInsert

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Nick Alexander :nalexander [he/him]]

Based on IRC feedback, this is just an oversight:

16:01 nalexander: wesj: I see that the Fennec ContentProvider's do not notifyChange in bulkInsert.  Is this intentional?
16:01 nalexander: It is causing trouble with better Sync/Fennec remote tabs integration, at least.
16:02 nalexander: mfinkle: ^
16:02 mfinkle: nalexander: assume it's not intentional
16:02 nalexander: mfinkle: done :)
16:08 wesj: nalexander: yeah, not intentional

Comment 1

[Mozilla RelEng Bot]

Try run for f11343d74cff is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f11343d74cff
Results (out of 33 total builds):
    success: 25
    warnings: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f11343d74cff

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Patch against m-i

I added robocop tests for one of the providers (BrowserProvider).  Testing notifications from other providers takes me long enough that I'd rather not do it if I don't have to.  

Each test could timeout.  I wait 1.5 seconds for an async notification from Android, which is not awesome, but I can't think of a better way to do it.  If this causes intermittent failures, perhaps we don't test any of this?

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Mozilla RelEng Bot from comment #1)
> Try run for f11343d74cff is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=f11343d74cff
> Results (out of 33 total builds):
>     success: 25
>     warnings: 8
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/nalexander@mozilla.
> com-f11343d74cff

Try build Mrc was green.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 703402 [details] [diff] [review]
Patch against m-i

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

Looks good. Probably should run try again with the CountDownLatch fix. We also have this waitForTest stuff for similar things (but I like the CountDownLatch) that uses a larger timeout (MAX_WAIT_MS) that I think ContentProviderTests should inherit. Its often helpful on try, with things that may be racy, to retrigger the test a few times.

::: mobile/android/base/tests/testBrowserProvider.java.in
@@ +1803,5 @@
> +            try {
> +                // ugly, but we need to wait for an asynchronous event
> +                // coming from Android, not from Gecko, so existing
> +                // helpers don't help
> +                latch.await(1500L, TimeUnit.MILLISECONDS);

I think we want to return latch.await, right? i.e. if this times out it should fail?

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Wesley Johnston (:wesj) from comment #4)
> Comment on attachment 703402 [details] [diff] [review]
> Patch against m-i
> 
> Review of attachment 703402 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Probably should run try again with the CountDownLatch fix. We
> also have this waitForTest stuff for similar things (but I like the
> CountDownLatch) that uses a larger timeout (MAX_WAIT_MS) that I think
> ContentProviderTests should inherit. Its often helpful on try, with things
> that may be racy, to retrigger the test a few times.
> 
> ::: mobile/android/base/tests/testBrowserProvider.java.in
> @@ +1803,5 @@
> > +            try {
> > +                // ugly, but we need to wait for an asynchronous event
> > +                // coming from Android, not from Gecko, so existing
> > +                // helpers don't help
> > +                latch.await(1500L, TimeUnit.MILLISECONDS);
> 
> I think we want to return latch.await, right? i.e. if this times out it
> should fail?

Good catch.  Forgot that `await(...)` doesn't throw on timeout.  Will re-try shortly.

Comment 6

[Nick Alexander :nalexander [he/him]]

> > I think we want to return latch.await, right? i.e. if this times out it
> > should fail?
> 
> Good catch.  Forgot that `await(...)` doesn't throw on timeout.  Will re-try
> shortly.

So, my tests passed only due to this error.  And it turns out that testing content observer notifications in JUnit 3 is not worth the effort.  How do you feel about landing this without tests, wesj?  In a way, Bug 740566 is a test that QA will hand verify.

Comment 7

[Wesley Johnston (:wesj)]

(In reply to Nick Alexander :nalexander from comment #6)
> > > I think we want to return latch.await, right? i.e. if this times out it
> > > should fail?
> > 
> > Good catch.  Forgot that `await(...)` doesn't throw on timeout.  Will re-try
> > shortly.
> 
> So, my tests passed only due to this error.  And it turns out that testing
> content observer notifications in JUnit 3 is not worth the effort.  How do
> you feel about landing this without tests, wesj?  In a way, Bug 740566 is a
> test that QA will hand verify.

I'm not really sure I understand why these can't pass here? What do these have to do with JUnit3? Why is "testing content observer notifications in JUnit 3 not worth the effort?

Comment 8

[Nick Alexander :nalexander [he/him]]

> I'm not really sure I understand why these can't pass here? What do these
> have to do with JUnit3? Why is "testing content observer notifications in
> JUnit 3 not worth the effort?

ContentProviderTest goes to some effort to provide a clean environment within the friendly confines of ActivityInstrumentationTestCase2.  I theorize that this environment is breaking content notifications, and chasing this down is really more effort than it's worth.

Strictly speaking, this is not a JUnit 3 thing -- it is an Android Instrumentation thing.  In Android Sync, we have a JUnit 4 test suite and a JUnit 3 instrumentation suite, and we use the JUnit versions to distinguish, but I should have been more specific.

I am going to hunt this down for a few more hours, but suffice it to say this is taking way more time than it's worth.  The notifications are getting fired, even if I can't determine the test suite voodoo needed to register a listener.

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #8)
> > I'm not really sure I understand why these can't pass here? What do these
> > have to do with JUnit3? Why is "testing content observer notifications in
> > JUnit 3 not worth the effort?
> 
> ContentProviderTest goes to some effort to provide a clean environment
> within the friendly confines of ActivityInstrumentationTestCase2.  I
> theorize that this environment is breaking content notifications, and
> chasing this down is really more effort than it's worth.

Aha!  MockContentResolver drops change notifications on the floor.  So that's interesting.

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #9)
> (In reply to Nick Alexander :nalexander from comment #8)
> > > I'm not really sure I understand why these can't pass here? What do these
> > > have to do with JUnit3? Why is "testing content observer notifications in
> > > JUnit 3 not worth the effort?
> > 
> > ContentProviderTest goes to some effort to provide a clean environment
> > within the friendly confines of ActivityInstrumentationTestCase2.  I
> > theorize that this environment is breaking content notifications, and
> > chasing this down is really more effort than it's worth.
> 
> Aha!  MockContentResolver drops change notifications on the floor.  So
> that's interesting.

Further confirmation:

http://stackoverflow.com/questions/11482104/how-to-junit-test-contentresolver-notifychange

We can extend MockContentResolver to record notifications, and I can update my suite of tests for this.  But is it Good for the Company?

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #10)

> We can extend MockContentResolver to record notifications, and I can update
> my suite of tests for this.  But is it Good for the Company?

Probably not for tests that aren't run on tbpl.

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> (In reply to Nick Alexander :nalexander from comment #10)
> 
> > We can extend MockContentResolver to record notifications, and I can update
> > my suite of tests for this.  But is it Good for the Company?
> 
> Probably not for tests that aren't run on tbpl.

The content provider tests are Mochitest Robocop tests.  Those tests do run on TBPL, right?  So it's down to "testing coverage" versus "time to implement and maintenance cost".

Comment 13

[Mozilla RelEng Bot]

Try run for 1d1fb65988c2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1d1fb65988c2
Results (out of 38 total builds):
    success: 29
    warnings: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1d1fb65988c2

Comment 14

[Mozilla RelEng Bot]

Try run for 1d1fb65988c2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1d1fb65988c2
Results (out of 39 total builds):
    success: 29
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1d1fb65988c2

Comment 15

[Nick Alexander :nalexander [he/him]]

Attachment: Updated patch against m-i

The tests for this are different enough, and I'm unfamiliar enough with the Fennec robocop test base, that re-review seems prudent.  Try build in progress.

Comment 16

[Mozilla RelEng Bot]

Try run for 1c8ebf7a4bcb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1c8ebf7a4bcb
Results (out of 36 total builds):
    success: 32
    warnings: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1c8ebf7a4bcb

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 705584 [details] [diff] [review]
Updated patch against m-i

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

Neat. Seems good.

::: mobile/android/base/db/GeckoProvider.java.in
@@ +306,5 @@
>              db.endTransaction();
>          }
> +
> +        if (rowsAdded > 0)
> +            mContext.getContentResolver().notifyChange(uri, null);

Since some of these run in a different process, I'm not sure this will ever be that useful for them, but I don't see any harm in doing it.

Comment 18

[Mozilla RelEng Bot]

Try run for 21ea639f2904 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=21ea639f2904
Results (out of 38 total builds):
    exception: 1
    success: 34
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-21ea639f2904

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Mozilla RelEng Bot from comment #18)
> Try run for 21ea639f2904 is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=21ea639f2904
> Results (out of 38 total builds):
>     exception: 1
>     success: 34
>     warnings: 3
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/nalexander@mozilla.
> com-21ea639f2904

All right, try run comes back Mrc green.  Not triggering multiple try runs since this is synchronous test code (previous iteration was asynchronous).  Landing this on m-i.

Comment 20

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9105fb1dcb0

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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