Closed Bug 958836 Opened 6 years ago Closed 6 years ago

Add robocop tests for Flash

Categories

(Firefox for Android :: Testing, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: snorp, Assigned: snorp)

Details

Attachments

(1 file, 2 obsolete files)

Right now we have no tests at all for Flash, so it's consistently busted. Let's fix that.
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 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.
Attachment #8358814 - Flags: review?(michael.l.comella)
Attachment #8358814 - Flags: review?(mark.finkle)
Attachment #8358814 - Flags: review+
(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.
(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 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.
(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
Attachment #8358814 - Attachment is obsolete: true
Attachment #8358814 - Flags: review?(michael.l.comella)
Attachment #8359429 - Flags: review?(michael.l.comella)
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
(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
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Status: NEW → ASSIGNED
Attachment #8359429 - Attachment is obsolete: true
Attachment #8359429 - Flags: review?(michael.l.comella)
Attachment #8359456 - Flags: review?(michael.l.comella)
Comment on attachment 8359456 [details] [diff] [review]
Add robocop test for Flash on Android

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

lgtm.
Attachment #8359456 - Flags: review?(michael.l.comella) → review+
https://hg.mozilla.org/mozilla-central/rev/2dc16f5371bd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.