780650 - Create Robocop API class to insulate reflection test code from Gecko internals

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Peterson [:cpeterson]]

This bug is forked from bug 778468 comment 33:

> (In reply to Chris Peterson (:cpeterson) from comment #29)
> > We could probably make Robocop more robust if we had Gecko classes dedicated
> > to  providing a stable API to thunk Robocop's dynamic class loader to
> > GeckoAppShell and friends. Robocop currently relies on fragile
> > implementation details of some Gecko method signatures that may change
> > because they are used by other Gecko classes. Gecko signature changes would
> > break the Robocop thunking classes at Fennec compile-time, not when running
> > Robocop tests on the tryserver.
> 
> +1. We should create a RobocopAPI class which is the only thing Robocop is 
> allowed to access via reflection, and that can reach into the rest of the code 
> as needed. Feel free to create a bug for that and throw it at me if you want.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add a RobocopAPI class and update robocop harness to use it

This part was fairly straightforward; I added a RobocopAPI class and updated the reflection-using code in build/mobile/robocop/ to use it instead of referencing various methods directly. GeckoEventListener, DrawListener, and PanningPerfAPI are still referenced directly.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - (WIP) Updated tests to not reach into fennec internals

And then I realized that was just the tip of the iceberg. The vast majority of reflection work happens in the tests/ folder itself, with lots of different tests grabbing lots of different classes by reflection and using them. I started cleaning up a couple of the simple ones in this WIP but was wondering if you had any thoughts on better ways to do this. If I keep doing this the RobocopAPI class is going to be huge, and I feel like there must be a better way.

Comment 3

[Chris Peterson [:cpeterson]]

Comment on attachment 650414 [details] [diff] [review]
Part 2 - (WIP) Updated tests to not reach into fennec internals

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

Maybe this is not a problem we can solve. If we had 100% test coverage, then our tests would (in theory) need to access 100% of our org.mozilla.gecko code.

As an alternative to an all-encompassing RobocopAPI class, we could just double-check that all classes and methods accessed by Robocop reflection are commented as such. If we wanted to get fancy, we could create a custom `@Robocop` annotation for classes and methods access from Robocop. The annotation would act as a comment for developers, but could also be checked when Robocop is running tests. That might be more clever than useful because the errors are still discovered at test-time, not compile-time.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 650411 [details] [diff] [review]
Part 1 - Add a RobocopAPI class and update robocop harness to use it

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

I think I'd like to land this first patch anyway, because in bug 781220 I'm trying to remove some of the dependencies on GeckoLayerClient and this makes it easier.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 650411 [details] [diff] [review]
Part 1 - Add a RobocopAPI class and update robocop harness to use it

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

LGTM with comments. Removing dependencies it good! :)

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +701,5 @@
>      public void setDrawListener(DrawListener listener) {
>          mDrawListener = listener;
>      }
>  
> +    /** Used by robocop for testing purposes. Not for production use! This is referenced via reflection by robocop. */

We can probably remove this comment about "referenced via reflection by robocop". We don't really need these warning comments if RobocopApi will give us friendly compile-time errors instead of test-time failures if the method signature changes.

The "testing purposes" comments are still useful, though.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Try run at https://tbpl.mozilla.org/?tree=Try&rev=439872eb4cc8 - the greenest try run I've ever seen!

Pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f707cf325d5

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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