938821 - Remove reflection from ContentProviderTest and extending classes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Reflection is used in setUp in order to get instances of the extending classes - we should just pass those classes in directly.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Spoke with Taylor on IRC.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Spoke with Taylor on IRC.

Comment 3

[:Margaret Leibovic]

FRI, hopefully bug 949208 will address at least part of this.

Comment 4

[Isura Edirisinghe]

Can I take this bug?

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Of course! :)

Let me know if you need a better description of what to do.

Comment 6

[Isura Edirisinghe]

Hi Michael,

In testHomeListsProvider there is reference to org.mozilla.gecko.db.HomeListsProvider. However I can't HomeLIstsProvider class in the source.

Comment 7

[:Margaret Leibovic]

(In reply to Isura Edirisinghe from comment #6)
> Hi Michael,
> 
> In testHomeListsProvider there is reference to
> org.mozilla.gecko.db.HomeListsProvider. However I can't HomeLIstsProvider
> class in the source.

It has been renamed to HomeProvider.java now, and it's functionality has changed quite a bit since that test was written, which is why it's disabled until we get the chance to update it. If you want to take on updating that test, you could go for it, but otherwise I would say you don't need to worry about fixing it.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Fixing testHomeListsProvider is bug 964464.

Isura, as Margaret mentioned, you don't need to worry about it (unless you want to!).

Comment 9

[Isura Edirisinghe]

Attachment: 938821.patch

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8368310 [details] [diff] [review]
938821.patch

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

We want to remove all instances of reflection, not just the instantiation.

I find reflection is easiest to find by looking for which reflection classes are imported by searching for ".reflect." or similar (e.g. `java.lang.reflect.Method`) and replace their uses with compile-time equivalents. Additionally, search for the `ClassLoader` class, which does not need to be imported.

Don't forget to remove the imports!

::: mobile/android/base/tests/ContentProviderTest.java
@@ +174,2 @@
>              String authorityField) throws Exception {
>          mProviderContract = mClassLoader.loadClass("org.mozilla.gecko.db.BrowserContract");

We should remove any use of ClassLoaders.

@@ +174,4 @@
>              String authorityField) throws Exception {
>          mProviderContract = mClassLoader.loadClass("org.mozilla.gecko.db.BrowserContract");
>          mProviderAuthority = (String) mProviderContract.getField(authorityField).get(null);
> +        mProviderClass = providerClass;

We shouldn't need to store this class - we should be able to call it directly. It seems the only use is newInstance(), which means we can just call `new ProviderClass(...);` instead.

Comment 11

[Isura Edirisinghe]

Okay it's clear now! I thought the bug was only referring to the TODO comment :)

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Remove java.lang.reflect imports from SessionTest.

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Remove getContentUri and get*Column from testHomeListsProvider.

This is rather artificial, since the test is currently disabled it
merely needs to compile.

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Remove ContentProviderTest.getContentUri.

Comment 15

[Nick Alexander :nalexander [he/him]]

Attachment: Part 4: Remove GUID constants.

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: Part 5: Remove loadContractInfo madness from testBrowserProvider.

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: Part 6: Remove loadContractInfo madness from testBrowserProviderPerf.

Comment 18

[Nick Alexander :nalexander [he/him]]

Attachment: Part 7: Remove ContentProviderTest.get*Column reflection.

Comment 19

[Nick Alexander :nalexander [he/him]]

Attachment: Part 8: Remove reflection from ContentProviderTest.appendUriParam.

Comment 20

[Nick Alexander :nalexander [he/him]]

Attachment: Part 9: Remove reflection from ContentProviderTest.authority.

Comment 21

[Nick Alexander :nalexander [he/him]]

Attachment: Part 10: Remove reflection from ContentProviderTest.

Comment 22

[Nick Alexander :nalexander [he/him]]

Isura: I'd like to apoligise for taking an assigned bug; I was working in the area, moved on this, and didn't realize you were already looking into it.

mcomella: these transformations are essentially mechanical.  They're broken into pieces for my sanity, if not yours.  The proof is in the pudding, a green try build:

https://tbpl.mozilla.org/?tree=Try&rev=48c4ace2b5ab

Comment 23

[Nick Alexander :nalexander [he/him]]

mcomella: you might find it easier to look at these commits (and the ones for Bug 969922) at:

https://github.com/ncalexan/gecko-dev/tree/bug-938821-remove-reflection

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372912 [details] [diff] [review]
Part 2: Remove getContentUri and get*Column from testHomeListsProvider.

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

r+ w/ comment.

::: mobile/android/base/tests/testHomeListsProvider.java
@@ +7,5 @@
>  
>  public class testHomeListsProvider extends ContentProviderTest {
> +	private static class Contract {
> +		public static final Uri CONTENT_URI = null;
> +		public static final Uri CONTENT_FAKE_URI = null;

Add a comment explaining that these are not init because they were added in a refactor where the disabled test only needed to compile.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372919 [details] [diff] [review]
Part 9: Remove reflection from ContentProviderTest.authority.

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

::: mobile/android/base/tests/testHomeListsProvider.java
@@ +32,5 @@
>      }
>  
>      @Override
>      public void setUp() throws Exception {
> +    	// This test is disabled, so this just needs to compile.

Nice, though I still think we should add the comment in patch #2's review as well.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372920 [details] [diff] [review]
Part 10: Remove reflection from ContentProviderTest.

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

r+ w/ nits.

::: mobile/android/base/tests/ContentProviderTest.java
@@ +173,5 @@
>  
> +	/**
> +	 * Factory function that makes new ContentProvider instances.
> +	 * <p>
> +	 * We want a fresh provider each test, so this is invoked in

nit: "...so this should be invoked...". This comment's existence does not make it so. ;)

@@ +176,5 @@
> +	 * <p>
> +	 * We want a fresh provider each test, so this is invoked in
> +	 * <code>setUp</code> before each individual test.
> +	 */
> +    protected static Callable<ContentProvider> sBrowserProviderCallable = new Callable<ContentProvider>() {

nit: Might just be splinter but indentation of javadoc. Tabs?

@@ +213,5 @@
>      }
>  
>      @Override
>      public void setUp() throws Exception {
> +        throw new Exception("You should call setUp(authority, databaseName) instead");

nit: UnsupportedOperationException

::: mobile/android/base/tests/testBrowserProviderPerf.java
@@ +9,5 @@
>  
>  import org.mozilla.gecko.GeckoProfile;
>  import org.mozilla.gecko.db.BrowserContract;
>  import org.mozilla.gecko.db.BrowserDB;
> +import org.mozilla.gecko.db.BrowserProvider;

nit: I don't think this import is needed.

::: mobile/android/base/tests/testDistribution.java
@@ +1,5 @@
>  package org.mozilla.gecko.tests;
>  
>  import org.mozilla.gecko.*;
>  import org.mozilla.gecko.db.BrowserContract;
> +import org.mozilla.gecko.db.BrowserProvider;

nit: I don't think this import is needed.

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8368310 [details] [diff] [review]
938821.patch

Isura, I apologize for setting this up as a mentor bug. I didn't realize it was so arduous! :(

If you want to continue working on the test framework, bug 943706 is available and, more challengingly, bug 970624.

Comment 28

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be5b8d98c85d
https://hg.mozilla.org/integration/mozilla-inbound/rev/387133d0df05
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd79dac304bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f920fcbc4d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8516ad95228
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4375f03b3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a83ace2b971
https://hg.mozilla.org/integration/mozilla-inbound/rev/611f026b232e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee591d0ba0b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e57a8a526d7

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be5b8d98c85d
https://hg.mozilla.org/mozilla-central/rev/387133d0df05
https://hg.mozilla.org/mozilla-central/rev/bd79dac304bb
https://hg.mozilla.org/mozilla-central/rev/1f920fcbc4d7
https://hg.mozilla.org/mozilla-central/rev/d8516ad95228
https://hg.mozilla.org/mozilla-central/rev/8b4375f03b3c
https://hg.mozilla.org/mozilla-central/rev/4a83ace2b971
https://hg.mozilla.org/mozilla-central/rev/611f026b232e
https://hg.mozilla.org/mozilla-central/rev/ee591d0ba0b6
https://hg.mozilla.org/mozilla-central/rev/7e57a8a526d7