958836 - Add robocop tests for Flash

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Right now we have no tests at all for Flash, so it's consistently busted. Let's fix that.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add robocop test for Flash on Android

Comment 2

[Geoff Brown [:gbrown]]

Comment on attachment 8358814 [details] [diff] [review]
Add robocop test for Flash on Android

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

::: mobile/android/base/tests/robocop.ini
@@ +64,5 @@
>  # disabled on x86 only; bug 907383
>  skip-if = processor == "x86"
>  # [testThumbnails] # see bug 813107
>  # [testVkbOverlap] # see bug 907274
> +[testAdobeFlash]

The list of tests is kept in alphabetical order, theoretically.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8358814 [details] [diff] [review]
Add robocop test for Flash on Android

>diff --git a/mobile/android/base/tests/testAdobeFlash.java b/mobile/android/base/tests/testAdobeFlash.java

>+            // Wait for confirmation of the pref change before proceeding with the test.
>+            final String[] prefNames = { "plugin.default.state" };
>+            final int ourRequestId = 0x7358;
>+            Actions.RepeatedEventExpecter eventExpecter = mActions.expectGeckoEvent("Preferences:Data");
>+            mActions.sendPreferencesGetEvent(ourRequestId, prefNames);

Where does the 0x7358 come from? Are you just making up an ID so we can grab it below? I'm just curious more than anything else.

I think this is a great, and simple, test for whether flash is working or not. Nice job.

I want to pass this to Mike for a second opinion on the latest API changes in the Test framework.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Geoff Brown [:gbrown] from comment #2)
> Comment on attachment 8358814 [details] [diff] [review]
> Add robocop test for Flash on Android
> 
> Review of attachment 8358814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/robocop.ini
> @@ +64,5 @@
> >  # disabled on x86 only; bug 907383
> >  skip-if = processor == "x86"
> >  # [testThumbnails] # see bug 813107
> >  # [testVkbOverlap] # see bug 907274
> > +[testAdobeFlash]
> 
> The list of tests is kept in alphabetical order, theoretically.

Oops, I missed that. But yeah.

I also assume we can't land this until we get the Flash APK installed during the testing.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 8358814 [details] [diff] [review]
> Add robocop test for Flash on Android
> 
> >diff --git a/mobile/android/base/tests/testAdobeFlash.java b/mobile/android/base/tests/testAdobeFlash.java
> 
> >+            // Wait for confirmation of the pref change before proceeding with the test.
> >+            final String[] prefNames = { "plugin.default.state" };
> >+            final int ourRequestId = 0x7358;
> >+            Actions.RepeatedEventExpecter eventExpecter = mActions.expectGeckoEvent("Preferences:Data");
> >+            mActions.sendPreferencesGetEvent(ourRequestId, prefNames);
> 
> Where does the 0x7358 come from? Are you just making up an ID so we can grab
> it below? I'm just curious more than anything else.

Yup. I just copy/pasted that code from another test and incremented the id.

Comment 6

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

Comment on attachment 8358814 [details] [diff] [review]
Add robocop test for Flash on Android

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

green.swf appears to be an empty file when I apply the patch - did you forget to `qref`? r+ after this is clarified.

I don't know how Flash HTML embedding works so I can't speak to robocop_adobe_flash.html, but everything else looks good. My suggestions may be overly cautious, so you don't *have* to change them, but I think it's best to be safe.

::: mobile/android/base/tests/robocop_adobe_flash.html
@@ +10,5 @@
> +codebase="http://fpdownload.macromedia.com/
> +pub/shockwave/cabs/flash/swflash.cab#version=8,0,0,0">
> +<param name="SRC" value="green.swf">
> +<embed src="green.swf" width="100" height="100">
> +</embed>

nit: I'd indent the <param> and <embed> tags since they're in the <object> tag.

@@ +11,5 @@
> +pub/shockwave/cabs/flash/swflash.cab#version=8,0,0,0">
> +<param name="SRC" value="green.swf">
> +<embed src="green.swf" width="100" height="100">
> +</embed>
> +</object> 

nit: Excess EOL whitespace.

::: mobile/android/base/tests/testAdobeFlash.java
@@ +26,5 @@
> +            mActions.sendGeckoEvent("Preferences:Set", jsonPref.toString());
> +
> +            // Wait for confirmation of the pref change before proceeding with the test.
> +            final String[] prefNames = { "plugin.default.state" };
> +            final int ourRequestId = 0x7358;

Assuming this is an arbitrary value as Finkle suspected (which it seems to be), I would make `ourRequestId` the value it is least likely to be in any normal use case. It starts at 1 [1] and only increments [2], so I would make it `Integer.MIN_VALUE`.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/PrefsHelper.java#28
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/PrefsHelper.java#47

@@ +34,5 @@
> +            JSONObject data = null;
> +            int requestId = -1;
> +
> +            // Wait until we get the correct "Preferences:Data" event
> +            while (requestId != ourRequestId) {

`GeckoEventExpecter.blockForEventData` times out and throws an assertion failure after 90s, however, in the worst (and to be fair, implausible) case, we can receive "Preferences:Data" continually until automation has to kill us after 60m. So, I think there should be a maximum number of checks here for "Preferences:Data" events, just to be on the (perhaps unreasonably) safe side.

@@ +36,5 @@
> +
> +            // Wait until we get the correct "Preferences:Data" event
> +            while (requestId != ourRequestId) {
> +                data = new JSONObject(eventExpecter.blockForEventData());
> +                requestId = data.getInt("requestId");

You should probably assert data.name/type/value are what you expect them to be.

@@ +40,5 @@
> +                requestId = data.getInt("requestId");
> +            }
> +            eventExpecter.unregisterListener();
> +
> +        } catch (Exception ex) { 

nit: EOL whitespace.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Michael Comella (:mcomella) from comment #6)
> Comment on attachment 8358814 [details] [diff] [review]
> Add robocop test for Flash on Android
> 
> Review of attachment 8358814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> green.swf appears to be an empty file when I apply the patch - did you
> forget to `qref`? r+ after this is clarified.

Apparently my git hg export alias didn't handle binaries. Fixed.

> 
> I don't know how Flash HTML embedding works so I can't speak to
> robocop_adobe_flash.html, but everything else looks good. My suggestions may
> be overly cautious, so you don't *have* to change them, but I think it's
> best to be safe.
> 
> ::: mobile/android/base/tests/robocop_adobe_flash.html
> @@ +10,5 @@
> > +codebase="http://fpdownload.macromedia.com/
> > +pub/shockwave/cabs/flash/swflash.cab#version=8,0,0,0">
> > +<param name="SRC" value="green.swf">
> > +<embed src="green.swf" width="100" height="100">
> > +</embed>
> 
> nit: I'd indent the <param> and <embed> tags since they're in the <object>
> tag.

Fixed

> 
> @@ +11,5 @@
> > +pub/shockwave/cabs/flash/swflash.cab#version=8,0,0,0">
> > +<param name="SRC" value="green.swf">
> > +<embed src="green.swf" width="100" height="100">
> > +</embed>
> > +</object> 
> 
> nit: Excess EOL whitespace.

Fixed

> 
> ::: mobile/android/base/tests/testAdobeFlash.java
> @@ +26,5 @@
> > +            mActions.sendGeckoEvent("Preferences:Set", jsonPref.toString());
> > +
> > +            // Wait for confirmation of the pref change before proceeding with the test.
> > +            final String[] prefNames = { "plugin.default.state" };
> > +            final int ourRequestId = 0x7358;
> 
> Assuming this is an arbitrary value as Finkle suspected (which it seems to
> be), I would make `ourRequestId` the value it is least likely to be in any
> normal use case. It starts at 1 [1] and only increments [2], so I would make
> it `Integer.MIN_VALUE`.
> 
> [1]:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> PrefsHelper.java#28
> [2]:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> PrefsHelper.java#47
> 

I just copied this stuff from another test, and there are several other places where people have done the same. I think we should punt those changes from this patch and file another bug to have a common "set this pref and wait for it to be applied" method for tests.

> @@ +34,5 @@
> > +            JSONObject data = null;
> > +            int requestId = -1;
> > +
> > +            // Wait until we get the correct "Preferences:Data" event
> > +            while (requestId != ourRequestId) {
> 
> `GeckoEventExpecter.blockForEventData` times out and throws an assertion
> failure after 90s, however, in the worst (and to be fair, implausible) case,
> we can receive "Preferences:Data" continually until automation has to kill
> us after 60m. So, I think there should be a maximum number of checks here
> for "Preferences:Data" events, just to be on the (perhaps unreasonably) safe
> side.

See above

> 
> @@ +36,5 @@
> > +
> > +            // Wait until we get the correct "Preferences:Data" event
> > +            while (requestId != ourRequestId) {
> > +                data = new JSONObject(eventExpecter.blockForEventData());
> > +                requestId = data.getInt("requestId");
> 
> You should probably assert data.name/type/value are what you expect them to
> be.

See above :)

> 
> @@ +40,5 @@
> > +                requestId = data.getInt("requestId");
> > +            }
> > +            eventExpecter.unregisterListener();
> > +
> > +        } catch (Exception ex) { 
> 
> nit: EOL whitespace.

Fixed

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add robocop test for Flash on Android

Comment 9

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

Comment on attachment 8359429 [details] [diff] [review]
Add robocop test for Flash on Android

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

::: mobile/android/base/tests/testAdobeFlash.java
@@ +45,5 @@
> +        }
> +
> +        blockForGeckoReady();
> +
> +        String url = getAbsoluteUrl("/robocop/robocop_adobe_flash.html");

Sorry that I missed it before but the URL can go into the m.a.b.tests.StringHelper class alongside the other robocop urls (e.g. [1]).

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/StringHelper.java#64

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Michael Comella (:mcomella) from comment #9)
> Comment on attachment 8359429 [details] [diff] [review]
> Add robocop test for Flash on Android
> 
> Review of attachment 8359429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/testAdobeFlash.java
> @@ +45,5 @@
> > +        }
> > +
> > +        blockForGeckoReady();
> > +
> > +        String url = getAbsoluteUrl("/robocop/robocop_adobe_flash.html");
> 
> Sorry that I missed it before but the URL can go into the
> m.a.b.tests.StringHelper class alongside the other robocop urls (e.g. [1]).
> 
> [1]:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/
> StringHelper.java#64

OK fixed

Comment 11

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #7)
> I just copied this stuff from another test, and there are several other
> places where people have done the same. I think we should punt those changes
> from this patch and file another bug to have a common "set this pref and
> wait for it to be applied" method for tests.

bug 959382.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add robocop test for Flash on Android

Comment 13

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

Comment on attachment 8359456 [details] [diff] [review]
Add robocop test for Flash on Android

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

lgtm.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc16f5371bd

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Also https://hg.mozilla.org/integration/mozilla-inbound/rev/df85e7787732 because I'm stupid

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2dc16f5371bd