757481 - Tests for prompt service

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Testing, defect, P3)

Description

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

I want to fix some stuff in the prompt service, but we have no tests making it a bit scary. So I'm writing some! This is a WIP to save my place.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Simple first step

This is super simple. Opens a dialog with a title and message, checks them, closes it. Also, it requires a bunch of reflection and which is ugly.

Its so simple, I think it should be the first test here, to be followed by more simples ones until we've got some good coverage. Leaving the WIP because testing the javascript part of this will be a different battle, but some basic tests like this will make me feel a lot better as we try and refactor out some ugliness here.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch 2

This adds a second test for a prompt with some list items on it. Turns out, things had to get significantly more complex (but found a bug in the first test?) 

We (currently) HAVE to call waitForReturn after showing a dialog to ensure we don't timeout and throw when we call sPromptQueue.offer(aReturn, 5, TimeUnit.SECONDS) in finish dialog. That means, we need to call it here (which is good anyway), but we need to call it on a thread that 1.) isn't the gecko ui thread and 2.) Isn't the testing thread (since we need to also call back). So we have to do a bit of a threading dance in here thats likely a bit fragile?

I'm starting to think maybe this isn't the way to go.

Comment 3

[Geoff Brown [:gbrown]]

Comment on attachment 746199 [details] [diff] [review]
Simple first step

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

Are there some additional, simple tests that could be added quickly? A multi-line prompt? Word wrap??

I pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2c65377aa5b8

testPrompts passed consistently, but testShareLink failed -- was my push bad, or is there some linkage there? Might be worth checking.

::: mobile/android/base/tests/testPrompts.java.in
@@ +19,5 @@
> +        blockForGeckoReady();
> +
> +        try {
> +            ClassLoader classLoader = getInstrumentation().getContext().getClassLoader();
> +            Class geckoAppClass = classLoader.loadClass("org.mozilla.gecko.GeckoApp");

Rather than access GeckoApp, etc directly, can you wrap in RobocopAPI? It's a silly thing, but RobocopAPI prevents accidental breakage caused when folks forget that some functions are accessed by reflection from the tests.

@@ +31,5 @@
> +            final Object ps = getPSMethod.invoke(getActivity());
> +            runOnUiThreadSync(new Runnable() {
> +                public void run() {
> +                    try {
> +                        mAsserter.dumpLog("got ps " + ps);

I would remove this dumpLog.

@@ +41,5 @@
> +            });
> +    
> +            mAsserter.ok(mSolo.waitForText(MY_PROMPT_TITLE), "Prompt has correct title", MY_PROMPT_TITLE);
> +            mAsserter.ok(mSolo.waitForText(MY_PROMPT_TEXT), "Prompt has correct message", MY_PROMPT_TEXT);
> +            mActions.sendSpecialKey(Actions.SpecialKey.BACK);

I like so see comments for the BACK key: "// Dismiss prompt for clean shutdown."?

Comment 4

[Geoff Brown [:gbrown]]

Comment on attachment 746637 [details] [diff] [review]
Patch 2

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

It *is* getting complicated. Not sure what to suggest...

::: mobile/android/base/tests/testPrompts.java.in
@@ +119,5 @@
> +            mSolo.clickOnText(button);
> +        }
> +
> +        // Now block while we wait for a result...
> +        waitForTest(new BooleanTest() {

If the thread throws an exception, mRes won't be set and you will wait here until you timeout. If the thread deadlocks or otherwise takes an exceptionally long time to complete, waitForTest will timeout and waitForResult will return, but you will still have an active thread, which might modify mRes. Instead of using waitForTest, couldn't you wait for the thread to end? Seems safer...

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v3

This gives up on the unit test style things and instead goes back to my old style. i.e. I load a page which has chrome privileges and call things like Services.prompt.alertCheck(). Java investigates the prompt and hits a button (ok, cancel, back, etc). Check the result in js and send something back to java if its good. Seems to work really well actually. I had to split this in two because my local tests were taking too long on device and causing robocop issues. I'm guessing try may show something similar...

https://tbpl.mozilla.org/?tree=Try&rev=c3cf85219051

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 811625 [details] [diff] [review]
Patch v3

Need to clean this up a little first actually.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: 1/3 Basic prompt tests

These are some very basic tests for the basic prompts that can be shown from html pages.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: 2/3 Prompt service tests

These are quite a bit more advanced, and test the nsIPromptService implementation in Fennec. Lots of flipping check boxes and making sure the right values are returned.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: 3/3 Select element tests

These are some basic tests for select elements (both single and multiselect). These could be written without roboextender, but my local firewall hates out test setup, and roboextender tests don't need it, so they run and pass fine.

Comment 10

[Geoff Brown [:gbrown]]

Comment on attachment 813454 [details] [diff] [review]
1/3 Basic prompt tests

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

Sorry for the long wait -- it has been a hectic week!

The test looks good to me.

I notice the output ends up looking something like:

2 INFO TEST-PASS | testPrompts | Loaded blank page - page title match
3 INFO TEST-PASS | testPrompts | Dialog title correct - The page at http://mochi.test
4 INFO TEST-PASS | testPrompts | Dialog message correct - Alert message
5 INFO TEST-PASS | testPrompts | Loaded blank page - page title match
6 INFO TEST-PASS | testPrompts | Dialog title correct - The page at http://mochi.test
7 INFO TEST-PASS | testPrompts | Dialog message correct - Alert message
8 INFO TEST-PASS | testPrompts | Loaded blank page - page title match
9 INFO TEST-PASS | testPrompts | Dialog title correct - The page at http://mochi.test

Debugging would be easier if the test id was reported for each page load.

I pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=656b09fd9a8e

It failed, as you warned it might -- changing to f+ for that. I do not know what the problem is. I am worried that some of the waitForText's might be called too early. That is, if some text "xxx" is displayed, you issue an action and then check to see if "xxx" is displayed, the test might continue too early, waitForText completed by the initial page, rather than the new one. But this is just a general, nagging feeling -- I cannot actually find an example.

::: mobile/android/base/tests/testPrompts.java.in
@@ +7,5 @@
> +import android.text.InputType;
> +
> +import org.json.JSONObject;
> +
> +public class testPrompts extends BaseTest {

I like a brief header comment that says what the scope of the test is.

@@ +21,5 @@
> +    private static final String VALUE = "value";
> +    protected static final String DEFAULT_TITLE = "The page at http://mochi.test";
> +    protected static final String ALERT_MSG = "Alert message";
> +    protected static final String CONFIRM_MSG = "Confirm message";
> +    protected static final String PROMPT_MSG = "Prompt message";

Maybe some of these should go in StringHelper?

@@ +97,5 @@
> +            });
> +        }
> +    }
> +
> +    protected void test(final int num) {

nit - s/test/loadTest/ or something else more descriptive

Comment 11

[Geoff Brown [:gbrown]]

Comment on attachment 813456 [details] [diff] [review]
2/3 Prompt  service tests

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

::: mobile/android/base/tests/testPromptService.java.in
@@ +7,5 @@
> +import android.text.InputType;
> +
> +import org.json.JSONObject;
> +
> +public class testPromptService extends BaseTest {

Header comment?

@@ +25,5 @@
> +    protected static final String DEFAULT_TITLE = "[Javascript Application]";
> +    protected static final String DEFAULT_CHECKMSG = "Checkbox";
> +    protected static final String PS_TITLE = "My Title";
> +    protected static final String URL = "chrome://roboextender/content/testPage.html#test";
> +    protected static final String BUTTON1 = "Button 2";

StringHelper?

@@ +106,5 @@
> +
> +        // test PromptService.select
> +        testSelect(59, PS_TITLE, OK, VAL1, VAL2);
> +        testSelect(60, PS_TITLE, OK, VAL1, VAL2);
> +        testSelect(61, PS_TITLE, BACK, VAL1, VAL2);

Wow -- lots of test cases here. Impressive!

@@ +140,5 @@
> +
> +        boolean start = check == CHECK.TRUE || check == CHECK.TRUE_TO_FALSE;
> +        mAsserter.is(start, mSolo.isCheckBoxChecked(text), "Checkbox " + text + " value = " + start);
> +        if (check == CHECK.TRUE_TO_FALSE || check == CHECK.FALSE_TO_TRUE) {
> +            mSolo.clickOnCheckBox(0);

Vulnerable to intermittent failure if another checkbox is displayed (perhaps in future). But sometimes this is the only way...

@@ +229,5 @@
> +        endDialog(button, false);
> +    }
> +
> +    // Test a checkbox at index, changes its value if endVal != null
> +    protected void editTextTest(final int index, final String startVal, final String endVal, int type, String hint) {

Some of this looks just like code in testPrompts -- is it worth a common base class, or could some of it find a home in an existing helper class?

Comment 12

[Geoff Brown [:gbrown]]

Comment on attachment 813457 [details] [diff] [review]
3/3 Select element tests

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

::: mobile/android/base/tests/roboextender/robocop_select.html
@@ +9,5 @@
> +  window[test]();
> +}
> +
> +function test1() {
> +  console.log("Robo: test1");

nit -- s/Robo/Robocop/

::: mobile/android/base/tests/testSelect.java.in
@@ +8,5 @@
> +import android.widget.CheckedTextView;
> +import android.widget.TextView;
> +import android.text.InputType;
> +import android.util.DisplayMetrics;
> +import android.util.Log;

unused import

@@ +34,5 @@
> +
> +        float top = mDriver.getGeckoTop()* + pos * dm.density;
> +        float left = mDriver.getGeckoLeft() + mDriver.getGeckoWidth() / 2;
> +
> +        mSolo.clickOnScreen(left, top);

I feel like I have seen this code in at least one other test...candidate for BaseTest or similar?

Comment 13

[Geoff Brown [:gbrown]]

Comment on attachment 813454 [details] [diff] [review]
1/3 Basic prompt tests

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

::: mobile/android/base/tests/robocop_prompts.html
@@ +39,5 @@
> +function test5() { testConfirm(false); } // Show confirm, hit back
> +function test6() { testAlert(NEW_VALUE); } // Show prompt with no value, change and hit ok
> +function test7() { testAlert(NEW_VALUE, VALUE); } // Show prompt with value, change and hit ok
> +function test8() { testAlert(null, VALUE); } // Show prompt with value, change and hit cancel
> +function test9() { testAlert(null, VALUE); } // Show prompt with value, change and hit back

I think you mean testPrompt here!

Comment 14

[Wesley Johnston (:wesj)]

Grr.. updated, but lots of blue:

https://tbpl.mozilla.org/?tree=Try&rev=7926ed2a24aa

I'm going to try splitting these up (more), and pushing them in chunks to see if that helps. Maybe we need rc4...

Comment 15

[Geoff Brown [:gbrown]]

That try run looks like good evidence for the need for rc4. Unless you find otherwise, you should probably file a bug like bug 905274.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3 - test Content prompts

This seemed fine on try now. Just lots of blue. Hopefully rc4 will help.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch 2/3 - Prompt Service tests

Split things up to all inherit from one base class. Also split up the PromptService tests, hoping it would help them not be so blue (it didn't).

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - Test select elements

Moved the clicking test code into BaseTest.

Comment 19

[Geoff Brown [:gbrown]]

Comment on attachment 818744 [details] [diff] [review]
Patch 1/3 - test Content prompts

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

Probably best to wait for rc4 and verify no blues before landing.

Comment 20

[Geoff Brown [:gbrown]]

I haven't forgotten about the second r?, but am pondering how to make these tests more robust.

https://tbpl.mozilla.org/?tree=Try&rev=7d89000692cb

Comment 21

[Geoff Brown [:gbrown]]

Comment on attachment 818752 [details] [diff] [review]
Patch 2/3 - Prompt Service tests

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

Wes -- Sorry to have left this so long. I looked at the try results too hastily and thought I saw intermittent failures, but it's much simpler: A consistent failure brought about by a string mismatch. In test 3, the string actually displayed is "Confirm message" (you can see this in the screenshots from the try run) while the test is expecting "Test confirm". I think if you just match up your prompt strings, all is well here. Also, rc4 is running now so you hopefully won't hit those retries. 

Fix the strings, show me a clean try run and you have an easy r+ waiting here.

Comment 22

[Geoff Brown [:gbrown]]

I think we've left this too long...not much value in keeping the bug open now.