Closed Bug 780650 Opened 8 years ago Closed 7 years ago

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


(Firefox for Android :: General, defect)

Not set



Firefox 17


(Reporter: cpeterson, Assigned: kats)



(2 files)

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.
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.
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.
Attachment #650414 - Flags: feedback?(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.
Attachment #650414 - Flags: feedback?(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]:

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.
Attachment #650411 - Flags: review?(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/
@@ +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.
Attachment #650411 - Flags: review?(cpeterson) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.