1199444 - Add test for UI/background events

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Test for bug 991167.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for BundleEventListener (v1)

Bug 991167 added BundleEventListener that allow dispatching an event
directly to the UI thread or to the background thread. This patch adds
tests to testEventDispatcher, to test that the events are dispatched on
the correct threads and that the Bundle messages are correct. Because
these events are asynchronous, the test waits for each event to be
processed before continuing.

Comment 2

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

Comment on attachment 8653727 [details] [diff] [review]
Add test for BundleEventListener (v1)

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

::: mobile/android/tests/browser/robocop/testEventDispatcher.java
@@ +39,5 @@
> +    private static final String UI_RESPONSE_EVENT = "Robocop:TestUIResponse";
> +    private static final String BACKGROUND_EVENT = "Robocop:TestBackgroundEvent";
> +    private static final String BACKGROUND_RESPONSE_EVENT = "Robocop:TestBackgrondResponse";
> +
> +    private static final long WAIT_FOR_EVENT_TIMEOUT = 10000; // 10 seconds

nit: -> WAIT_FOR_BUNDLE_EVENT_TIMEOUT_MILLIS

This is only used for bundle listeners.

@@ +85,5 @@
> +        final long startTime = System.nanoTime();
> +        while (!handledAsyncEvent) {
> +            try {
> +                wait(WAIT_FOR_EVENT_TIMEOUT);
> +                if (System.nanoTime() - startTime + 1e8 // allow 100ms of leeway.

Why is 100ms of leeway necessary? Why not incorporate this into the WAIT_FOR_EVENT_TIMEOUT value?

@@ +106,5 @@
>          GeckoHelper.blockForReady();
>          NavigationHelper.enterAndLoadUrl(mStringHelper.getHarnessUrlForJavascript(TEST_JS));
>  
>          js.syncCall("send_test_message", GECKO_EVENT);
> +        fAssertTrue("Should have handled Gecko event synchronously", handledGeckoEvent);

For future reference, it'd be good to put these cleanups (as opposed to the BundleEventListener changes) in a separate patch to make it easier to follow the motivation for these changes.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Michael Comella (:mcomella) from comment #2)
> Comment on attachment 8653727 [details] [diff] [review]
> Add test for BundleEventListener (v1)
> 
> Review of attachment 8653727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +85,5 @@
> > +        final long startTime = System.nanoTime();
> > +        while (!handledAsyncEvent) {
> > +            try {
> > +                wait(WAIT_FOR_EVENT_TIMEOUT);
> > +                if (System.nanoTime() - startTime + 1e8 // allow 100ms of leeway.
> 
> Why is 100ms of leeway necessary? Why not incorporate this into the
> WAIT_FOR_EVENT_TIMEOUT value?

It's to guard against situations where, e.g., we wait for 10s, but our measured time difference was 9.99s due to clock discrepancies, differences in wait() implementation, etc. The docs don't really say whether this could happen though.

I think I'll just change it to something like wait for 1s at a time, and just wait for multiple times to get to the timeout.

Comment 4

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

While I can see how this could happen, I've never heard of this before – perhaps it's not an issue in Java?

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2956e60f825

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b2956e60f825