Reflection is used to change the access levels (e.g. protected, public) on certain members of this class. Really, the class should be declared protected, extended by a mock class exclusive to testing, and accessors to these otherwise hidden fields added to this mock.

I did the easy stuff. It seems difficult to remove the later reflection using a MockBrowserSearch since BrowserSearch is constructed in BrowserApp.onCreate, which means BrowserApp would need to be mocked, which means we're testing a different Activity and the majority of the code in BaseTest.setUp would have to be duped in a sub-class. BrowserApp.onCreate calls BrowserSearch.newInstance statically, which means this could be replaced with a call to a BrowserSearchFactory.newInstance, where newInstance will return either a regular BrowserSearch instance or an overriding mock class. What do you think?

Comment on attachment 8342743 [details] [diff] [review] Part 1: Create SuggestClient without reflection. Nevermind, I apparently forgot to compile. x_x This is harder than I thought because SuggestClient is package private. Should we make it public? Note to future travellers: do not remove the @RobocopTarget annotation.

Comment on attachment 8342743 [details] [diff] [review] Part 1: Create SuggestClient without reflection. Review of attachment 8342743 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. Are there more parts to come? ::: mobile/android/base/home/SuggestClient.java @@ -54,5 @@ > mCheckNetwork = true; > } > > /** > * This constructor is used exclusively by Robocop. This comment says what @RobocopTarget enforces. Explain why there's a different constructor or remove the comment.

(In reply to Michael Comella (:mcomella) from comment #1) > Created attachment 8342743 [details] [diff] [review] > Part 1: Create SuggestClient without reflection. > > I did the easy stuff. > > It seems difficult to remove the later reflection using a MockBrowserSearch > since BrowserSearch is constructed in BrowserApp.onCreate, which means > BrowserApp would need to be mocked, which means we're testing a different > Activity and the majority of the code in BaseTest.setUp would have to be > duped > in a sub-class. > > BrowserApp.onCreate calls BrowserSearch.newInstance statically, which means > this could be replaced with a call to a BrowserSearchFactory.newInstance, > where newInstance will return either a regular BrowserSearch instance or an > overriding mock class. > > What do you think? So, this approach seems more hairy than it's worth. Is there a reason to not allow Robocop to set the client directly with something like BrowserSearch.setSuggestionClient()? We already have work-arounds in place, like at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#507. We document that setSuggestionClient is private and for Robocop and then we move on. What am I missing?

> Is there a reason to not allow Robocop to set the client directly with > something like BrowserSearch.setSuggestionClient()? We already have > work-arounds in place, like at > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/ > BrowserSearch.java#507. And maybe we only allow setting if the existing value is null, to prevent bad things.

Did as recommended in comment 4 and comment 5.

Comment on attachment 8375898 [details] [diff] [review] Create and set SuggestClient in testing without reflection. Review of attachment 8375898 [details] [diff] [review]: ----------------------------------------------------------------- If it's green locally and on try, it looks good to me. ::: mobile/android/base/home/BrowserSearch.java @@ +547,5 @@ > + /** > + * Sets the private SuggestClient instance. Should only be use for testing purposes. > + */ > + @RobocopTarget > + public void setSuggestClient(final SuggestClient client) { Consider using this in whatever code sets the suggest client already.

green on try after taking the suggestions in comment 7: https://tbpl.mozilla.org/?tree=Try&rev=13c63ab92f52

Comment on attachment 8376361 [details] [diff] [review] Create and set SuggestClient in testing without reflection. Review of attachment 8376361 [details] [diff] [review]: ----------------------------------------------------------------- Move r+.

https://hg.mozilla.org/integration/fx-team/rev/0beafa155ee9 Goodbye, reflection.