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

Spoke with Taylor on IRC.

Spoke with Taylor on IRC.

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

Can I take this bug?

Of course! :) Let me know if you need a better description of what to do.

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

(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.

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

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.

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

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

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

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 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 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 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 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.

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

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